Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix running precompiled tests via ginkgo CLI on windows (#529) #531

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olegch
Copy link

@olegch olegch commented Oct 27, 2018

Hello,

Here is a suggested change to support running precompiled tests on windows via ginkgo CLI (see issue #529 for more details).

@williammartin
Copy link
Collaborator

Change mostly makes sense to me. I wonder if we should start running CI against windows now (https://blog.travis-ci.com/2018-10-11-windows-early-release), I'd like to get some coverage if we are going to be supporting other environments like this.

var packageName string
if strings.HasSuffix(path, ".test.exe") {
packageName = strings.TrimSuffix(filepath.Base(path), ".test.exe")
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move assign value in else block to packageName declaration?

@blgm
Copy link
Collaborator

blgm commented Aug 19, 2019

One thing that could be improved is to use Go standard library functions that automatically append .exe on Windows. For instance when the exec.LookPath function is called with a path name containing a / (or \ on Windows) it will look for files with a .exe extension. The advantage of this is separation of concerns, and also less code to maintain.

@johnSchnake
Copy link

Checking on the status of this. Happy to help if needed.

I had originally thought that just getting rid of the exe suffix would have been sufficient but was surprised in my testing to find thats not the case.

@onsi
Copy link
Owner

onsi commented May 27, 2021

sorry y'all i haven't paid enough attention to this issue @johnSchnake can you confirm that this change fixes support for precompiled tests on windows? or are you saying they it doesn't?

i don't have access to a windows box to test it on myself...

@johnSchnake
Copy link

Sure thing. I just tried it out and was hacking a bit but when I rebased this onto master, put the exe extension back on my the tests, and ran on an azure k8s cluster I could see tests starting.

prior to all that jazz the master branch would just report no suites found.

A rebase may be all this needs unless you know of other shortcomings I hadn’t seen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants