Fix tests for nomad/client/driver/spawn package to work on Windows. #442
Conversation
| switch cmd := os.Args[1]; cmd { | ||
| case "echo": | ||
| fmt.Println(strings.Join(os.Args[2:], " ")) | ||
| case "sleep": |
There was a problem hiding this comment.
Nice. I was trying to figure out how to do this on Windows and I found a bunch of hacks, but this one should be fairly portable in our case.
| } | ||
| } | ||
|
|
||
| func TestSpawn_SetsLogs(t *testing.T) { | ||
| // TODO: Figure out why this test fails. If the spawn-daemon directly writes |
There was a problem hiding this comment.
Out of curiosity was the fix doing the Close?
There was a problem hiding this comment.
I am not sure. I just checked out master at a point before these changes and removed the call to t.Skip. With that code go test fails on my machine because "echo" isn't available on the path.
C:\nomadproject\src\github.com\hashicorp\nomad\client\driver\spawn>git describe
v0.1.2-615-g9db262f
C:\nomadproject\src\github.com\hashicorp\nomad\client\driver\spawn>go test -v -run SetsLogs
=== RUN TestSpawn_SetsLogs
--- FAIL: TestSpawn_SetsLogs (0.03s)
spawn_test.go:60: Spawn() failed: Failed to execute user command: Error starting user command: exec: "echo": executable file not found in %PATH%
FAIL
exit status 1
FAIL github.com/hashicorp/nomad/client/driver/spawn 0.118s
So I would say the fix was adding TestMain and using the binary constructed by go test as a self packaged portable echo.
But the TODO comment seems to indicate that you had experienced a different type of failure where the echo command could be found—maybe go test was running in a Cygwin or msys shell—but couldn't write to the log file for some reason. So the problem doesn't seem to reproduce the same way for me.
|
Looks good. Want to see what you do to make the TestMain/appMain more general. This is a problem even for client/task_runner/alloc_runner tests. |
|
There is a pattern for TestMain/appMain forming. I expect we can eventually factor out a DRY version of appMain to reuse across all the tests that need portable test tasks, but I'd rather make a new PR for that later, once I've got all the use cases figured out. |
|
LGTM |
Fix tests for nomad/client/driver/spawn package to work on Windows.
|
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.
Changes to improve test portability:
os.Removeworks on Windows.TestSpawn_SetsLogson Windows, as it passes now.Other changes:
defer f.Close()calls after theif err != nil {...}check.t.Parallel()in tests to reduce test run time.