-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 tests for nomad/client/driver/executor package to work on Windows. #462
Conversation
It looks to me like the test failures were caused by an unrelated part of the code, something to do with the client/fingerprint package. |
Shouldn't this line also use the TestMain: https://github.com/hashicorp/nomad/pull/462/files#diff-4ec3b758ae80396d0810cd04db40e05eL235 |
@@ -14,6 +15,57 @@ import ( | |||
"github.com/hashicorp/nomad/nomad/structs" | |||
) | |||
|
|||
func TestMain(m *testing.M) { | |||
switch os.Getenv("TEST_MAIN") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment that we switch the behavior depending on the environment variable and why we do this.
Thanks for the review comments above. I agree with everything and will update this PR when I get a chance. There is a very good reason why |
OK. I have rebased and added a commit based on the above review comments. I have not yet factored the appMain code into a separate package. Would you prefer I do that and update this PR and #442 before we merge? It seems easier to factor out the common code later to avoid a dependency between the PRs, but I am happy to do it either way. |
This is actually not the point of that test. If you look here: https://github.com/hashicorp/nomad/pull/462/files?diff=split#diff-4ec3b758ae80396d0810cd04db40e05eR337 you can see that the alloc directory is destroyed which removes the task state information. So what the test is really checking is that an open to with invalid state will return an error. |
As for the shared code between the two PRs, I am okay with merging these in and having a follow up PR. Will wait for an update from you on that one test and then merge! |
Windows/OS X seem to work but linux is failing: http://pastebin.com/PeeGwD20 |
Oops, I was thinking of As for failing on Linux, I will have to reproduce and investigate. |
I was wondering why Fixing that oversight as well as using |
All of the tests pass for me on Windows as well as Ubuntu 15.10 (inside VitualBox). The TravisCI test passes for the client/driver/executor package, |
@dadgar please take another look. |
Closing in favor of #497 |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
With these changes, all tests for this package pass on Windows.