Fixing tests for Windows #2927
Fixing tests for Windows #2927
Conversation
Fixing the config/config_test, the discovery/file/file_test and the promql/promql_test tests for Windows. For most of the tests, the fix involved correct handling of path separators. In the case of the promql tests, the issue was related to the removal of the temporal directories used by the storage. The issue is that the RemoveAll() call returns an error when it tries to remove a directory which is not empty, which seems to be true due to some kind of process that is still running after closing the storage. To fix it I added some retries to the remove of the temporal directories. Adding tags file from Universal Ctags to .gitignore
Renaming the name of a file of the config tests, in order to properly use the Go build tags feature.
| @@ -13,6 +13,7 @@ | |||
| .*.swo | |||
| *.iml | |||
| .idea | |||
| tags | |||
There was a problem hiding this comment.
At least for me 😉, I use Exuberant CTags during development to navigate the code. I can remove this if you like.
config/config_notwin_test.go
Outdated
| @@ -0,0 +1,28 @@ | |||
| // Copyright 2017 The Prometheus Authors | |||
There was a problem hiding this comment.
Could we name it config_unix_test.go?
http://blog.ralch.com/tutorial/golang-conditional-compilation/ also says we don't need to mention build tags if the files are named right.
There was a problem hiding this comment.
I think not, because unix is not a $GOOS valid value. BTW I got inspiration to use this name from here: $GOROOT/src/internal/testenv
Of course, it could be changed, but I'll need to add a list of all the unix like operating systems instead of using the !windows. Here is an example of your recommendation: $GOROOT/src/os/env*test.go_
If you still think that the approach follow by env*test.go_ is better, please let me know and I'll be glad to change my implementation.
There was a problem hiding this comment.
Got it, sorry you are right. I just don't like the notwin in the name. We could do config_default_test.go config_win_test.go like here: https://golang.org/src/cmd/dist/ (see sys_*).
| @@ -84,15 +87,20 @@ func NewCallbackCloser(fn func()) Closer { | |||
| } | |||
|
|
|||
| func (t temporaryDirectory) Close() { | |||
| retries := temporaryDirectoryRemoveRetries | |||
There was a problem hiding this comment.
Does the retrying actually help in practice?
There was a problem hiding this comment.
Yes, it helps. In a Windows machine, the promql tests always fail due to RemoveAll() can't remove a directory because is not empty. The strange thing is that if you go to the offending directory after the tests finished, the directory is actually empty.
My first approach to fix this, was to check if the Storage was properly close before trying to delete the storage directory, but I couldn't find any issue in that (all the proper channels seems to be wait before returning the control to the calling function). Then I checked, if was LevelDB, but I didn't have any luck founding the issue. So, maybe this is a bug of RemoveAll() in Windows, maybe Prometheus is giving back the control before all the things are properly close or maybe LevelDB is the one that is giving back the control before it should.
Fixing the config/config_test, the discovery/file/file_test and the promql/promql_test tests for Windows. For most of the tests, the fix involved correct handling of path separators. In the case of the promql tests, the issue was related to the removal of the temporal directories used by the storage. The issue is that the RemoveAll() call returns an error when it tries to remove a directory which is not empty, which seems to be true due to some kind of process that is still running after closing the storage. To fix it I added some retries to the remove of the temporal directories.
Adding tags file from Universal Ctags to .gitignore