Fix tests for Windows - part 1 #8414
Conversation
There was a problem hiding this comment.
Hey Pawel, thanks for tackling this! I've added some comments, but I have one more general one. I think you have four categories of patches here:
- Build-system related (makefile, appvayor, gitattributes)
- Actual fixes of tests
- Temporary deactivation of tests that really should be fixed
- Deactivation of tests that make no sense on Windows
Could you please reorganize them such that you have four patches, one per category? This eases bug-tracing and reviewing I think.
Furthermore, your Makefile change makes the tests on Darwin fail and I have no clue if those changes are valid. @reimda could you take a look at the changes to .gitattributes, Makefile and appveyor.yml!?
Could you please split the patches into
| // leaving port set to zero to let kernel pick | ||
| return &net.TCPAddr{IP: naddr.IP}, nil | ||
| //need to choose IPv4 address | ||
| if len(naddr.Mask) == net.IPv4len { |
There was a problem hiding this comment.
I don't understand this change. Why do we special case IPv4 here? Is this a generic fix or specific to windows? It looks like it will break IPv6 users.
There was a problem hiding this comment.
You are completely right. Here we have much deeper problem, I opened an issue for it: #8451. I will rollback this change in http_response.go. Till it is not fixed, I will disable http_response_test.go for Windows.
There was a problem hiding this comment.
Thanks. Good idea separating this out
internal/globpath/globpath_test.go
Outdated
| @@ -1,3 +1,7 @@ | |||
| // +build !windows | |||
|
|
|||
| // TODO: should be enabled for Windows when Glob related issues for Windows are fixed | |||
There was a problem hiding this comment.
any idea what these failures are?
[edit] just curious. feel free to ignore.
There was a problem hiding this comment.
@ssoroka Yes, super asterisk doesn't work for Windows, probably because there is bug in internal/globpath/globpath.go.
I put more information here: #6248 (comment)
Fixing that issue in internal/globpath/globpath.go will enable super asterisk for Windows for these plugins:
- file
- filecount
- filestat
- logparser
- phpfpm
- tail
| go test -short $(race_detector) ./plugins/inputs/procstat/... | ||
| go test -short $(race_detector) ./plugins/inputs/ntpq/... | ||
| go test -short $(race_detector) ./plugins/processors/port_name/... | ||
| go test -short ./... |
There was a problem hiding this comment.
Any reason we dropped the race detector?
There was a problem hiding this comment.
Build with race detector temporary enabled: https://ci.appveyor.com/project/influx/telegraf/builds/36450543
go test -short -race ./...
# runtime/cgo
exec: "gcc": executable file not found in %PATH%
? github.com/influxdata/telegraf [no test files]
FAIL github.com/influxdata/telegraf/agent [build failed]
FAIL github.com/influxdata/telegraf/config [build failed]
Probably related to golang/go#27089
Few days ago I tried with mingw installed https://ci.appveyor.com/project/influx/telegraf/builds/36325329
make test-windows
go test -short -race ./...
go build runtime/cgo: C:\go\pkg\tool\windows_amd64\cgo.exe: exit status 2
? github.com/influxdata/telegraf [no test files]
FAIL github.com/influxdata/telegraf/agent [build failed]
FAIL github.com/influxdata/telegraf/config [build failed]
But it works fine on my local Windows.
@ssoroka if you have any idea how to configure CI for Windows to run tests with -race let me know.
…bpath itself needs to be fixed)
…ting proper comments)
…esn't work on CI)
… issues with time on Windows and return of lookupAddr function from 'net' package)
…es with time on Windows)
|
Right now there are following tests which are disabled on Windows:
|
Required for all PRs:
Fixing short unit tests for Windows
Based on: #6255
Should fix: #3747