Windows tests fixing by vlastahajek · Pull Request #6255 · influxdata/telegraf

vlastahajek · 2019-08-14T13:33:26Z

Creating PR with intermediate fixes for various tests on Windows, reported in #3747, to validate fixes.

Required for all PRs:

Signed CLA.

…ar in comparison

…orms via converting them to slash separated (unix style) path

…t supported on Windows

…upported on windows

…path separator

…line split method for Windows

…platform agnostic

vlastahajek · 2019-08-14T13:35:45Z

I did first set of fixes for tests on Windows. I had to touch some internal components. Please, review.
More will come later.

danielnelson · 2019-08-14T17:38:08Z

logger/logger_test.go

 	SetupLogging(config)
 	log.Printf("E! TEST")
-	log.Printf("I! TEST") // <- should be ignored
+	log.Printf("I! TEST") // <- should be ignoredgo


danielnelson · 2019-08-14T17:40:14Z

plugins/inputs/disk/disk_test.go


 	tags1 := map[string]string{
-		"path":   "/",
+		"path":   string(os.PathSeparator),


Tell me a bit about this, we aren't actually running the plugin on the filesystem right? Why do we need to modify the path separators?

Generally, go filesystem related functions on Windows can take Unix style paths, but return Windows style (problem also for globpath).
And this line sets path with \.

Ahh, can we use "path": filepath.Clean("/") for these?

filepath.Clean("/") could the work, but seems a bit magic on the places when we need to construct a OS specific file path.

danielnelson · 2019-08-14T17:45:49Z

plugins/inputs/exec/exec.go

-		return nil, nil, fmt.Errorf("exec: unable to parse command, %s", err)
+	var split_cmd []string
+	var err error
+	//cannot use shellquote.Split on Windows as it threats Windows path separator char '\' as a mark for backslashed special char


Honestly, I love to remove shellquote completely and use a list of args on all platforms. That said I think this change has a high likelyhood of breaking existing config files.

I think instead we should skip associated failing tests in Windows and when -short is specified.

I don't understand.
When I had discovered this problem I was wondering how can be that exec plugin actually works for anyone on Windows. Only condition it can work now is that there is no path in the command.
How this fix would break existing config files?

How this fix would break existing config files?

It come down to:

I was wondering how can be that exec plugin actually works for anyone on Windows

Anyone on Windows using this probably has a very precise command string here with unix style quoting. If the behavior is changed in anyway, it will break their config. The only way this wouldn't be true is if this 100% can't work at all in Windows.

Separate from this PR, we could add a new array based command option, similar to the exec output.

danielnelson · 2019-08-14T17:51:11Z

testutil/testutil.go

+
+func init() {
+	if runtime.GOOS == "windows" {
+		LineSeparator = "\r\n"


This isn't really a Windows vs Unix thing, it's more about file encoding or terminal setup. I'd like to fix this more close to the tests themselves, not have this a package level item. I'll try to give more explanation by the instances where it is used.

In tests, it is the matter of git checkout behavior. Default setting on Windows is that text files are converted.
What about adding this line the .gitattributes:
**/testdata/** test eol=lf
I tested that and it works smooth. I could just revert all related changes. It seems to be the best solution for this.
Only downside is that people, mainly Windows developers, have to aware of this and do not count with CRLF. Luckily, there is just a few pure Windows tests and they don't deal with line endings.

danielnelson · 2019-08-14T17:55:04Z

internal/config/config_test.go

 	assert.Equal(t, internal.Size{Size: 1024 * 1024}, inputHTTPListener.MaxBodySize)
 	// Tests toml multiline basic strings.
-	assert.Equal(t, "/path/to/my/cert\n", inputHTTPListener.TLSCert)
+	assert.Equal(t, "/path/to/my/cert"+testutil.LineSeparator, inputHTTPListener.TLSCert)


We don't know what the line separator is, it depends on how git checked out the code. We should update this test to trim the string and then do the comparison. Elsewhere we should test that we can load a cert with either line ending.

Actually I originally did trimming here. But than I encountered the one bellow and changed that this way.

danielnelson · 2019-08-14T18:07:09Z

internal/internal_test.go


 	assert.NoError(t, err)
-	assert.Equal(t, "foo\n", string(out))
+	assert.Equal(t, "foo"+testutil.LineSeparator, string(out))


I've mixed feelings on if these tests should just be deleted, but I see testing it proper would be a fair bit of work. We could instead run a command using the GO_WANT_HELPER_PROCESS method and then we can determine the line separator.

danielnelson · 2019-08-14T18:18:11Z

internal/globpath/globpath.go

 		hasMeta:      hasMeta(path),
 		HasSuperMeta: hasSuperMeta(path),
-		path:         filepath.FromSlash(path),
+		path:         path,


Will this change the style of the paths returned? If so this could be a breaking change and we need to be very careful. Please double and triple check the changes in this file.

With fixing other test I had to make additional changes. All the fixes lead to the state where paths are always compared in the same form (forward slash as a path separator). It will not change any path returned

…paths must be in the same form (with slash as a path separator)

…h separator

…h in the map

…nting also CR char

…o CR char and comparing different MD5 sum

… dir and composing sock path using OS specific path separator char

…host by filtering IPv4 address

…le extension

… separator

…or Windows

…ng timeout

… for Windows

… OS specific error message

…or Windows

…s not for Windows

…l comparision as it can happen very fast

…ng pre-created sock file and skipping unixgram test as unixgram is not support on Windows

…r Windows

…Windows path to file URL

…th for file URL always to slash separated and skipping chmod on Windows

vlastahajek · 2019-09-05T14:48:02Z

I continued fixing rest of the tests to know what issues are there, still without a clean way how to properly deal with line and paths separators.
Now all tests finish either in success state or they are skipped because its feature is not available on Windows.
Let's continue by discussing specific issues.
I will update tests and clean git history when settled.

…ash forward separator

…path

…command line split method for Windows" This reverts commit 046ed44

danielnelson · 2019-09-13T18:35:50Z

Can you also update the appveyor.yml to run more of the tests?

…ey are not supported on older Windows (< Windows 10 build 17063)

… specific error message

…g to be platform agnostic" This reverts commit b5b5519

…s by counting also CR char" This reverts commit 7da21e7

vlastahajek · 2019-09-17T08:04:19Z

Done. All the short tests run now.

After discovering that Appveyor environment is bit different, mainly git is probably configured to do not convert line ending, I pushed proposed solution using a rule in main.gitattributes files, to force lf' line ending always.
Another fact on Windows is that AF_UNIX socket family is supported only since Windows 10 build 17063, so I made those test skipped on Windows generally.

sjwang90 · 2020-11-18T20:59:13Z

PR #8414 covers more of this PR. A few tests (which will be still disabled after PR #8414 is merged) will need some changes in production code - some of the example of these changes are included in this PR.

We'll close this PR once those examples are included.

sspaink · 2021-03-26T18:08:05Z

After #8414 has been closed, I think this can now be closed as well.

vlastahajek and others added 10 commits August 2, 2019 14:32

Fixing TestConfig_LoadSpecialTypes on Windows by omitting line end ch…

60f2999

…ar in comparison

Merge branch 'master' into vh-windows-tests-fixing

6ea77bd

Fix influxdata#3747: fixed TestWriteToTruncatedFile on Windows

3396c0b

Fix influxdata#3747: Making dealing with paths consitent across platf…

513e47a

…orms via converting them to slash separated (unix style) path

Fix influxdata#3747: Skipping bcache test on Windows, as bcache is no…

57ebc93

…t supported on Windows

Fix influxdata#3747: Skipping ceph tests on windows, as ceph is not s…

716c799

…upported on windows

Fix influxdata#3747: Fixing disk tests: Expected paths depends on OS …

b2fb21d

…path separator

Fix influxdata#3747: Fixing exec on Windows: Using different command …

046ed44

…line split method for Windows

Fix influxdata#3747: Fixing exec tests: Adding Windows specific command

3e71317

Fix influxdata#3747: Fixing string comparison with line ending to be …

b5b5519

…platform agnostic

Fix influxdata#3747: Fixing exec tests

676754d

danielnelson suggested changes Aug 14, 2019

View reviewed changes

vlastahajek added 17 commits August 27, 2019 11:11

Merge branch 'master' into vh-windows-tests-fixing

7357de9

Fix influxdata#3747: To provide os independent filepath matching all …

872b678

…paths must be in the same form (with slash as a path separator)

Fix influxdata#3747: Fixing filecount tests by using OS dependent pat…

8a561fb

…h separator

Fix influxdata#3747: Fixing filesystem_helper to look for unified pat…

8337664

…h in the map

Fix influxdata#3747: Fixing filesystem_helper tests on Windows by cou…

7da21e7

…nting also CR char

Fix influxdata#3747: Fixing filestat tests on Windows by counting als…

7812e69

…o CR char and comparing different MD5 sum

Fix influxdata#3747: Fixing haproxy test by getting OS dependent temp…

85bb982

… dir and composing sock path using OS specific path separator char

Fix influxdata#3747: Fixing http_response on Windows binding to local…

c341c85

…host by filtering IPv4 address

Fix influxdata#3747: Fixing leofs tests on Windows by adding executab…

c04c9a4

…le extension

Fix influxdata#3747: Fixing logparser tests by using OS specific path…

ff39d15

… separator

Fix influxdata#3747: Skipping lustre2 tests on Windows as it is not f…

e73b288

…or Windows

Fix influxdata#3747: Fixing net_response tests on Windows by increasi…

8ca582b

…ng timeout

Fix influxdata#3747: Skipping passenger tests on Windows as it is not…

5345a6b

… for Windows

Fix influxdata#3747: Fixing phpfpm tests on Windows by comparing with…

a372cb4

… OS specific error message

Fix influxdata#3747: Skipping postfix tests on Windows as it is not f…

96007b1

…or Windows

Fix influxdata#3747: Skipping tests using unixgram on Windows as it i…

41592af

…s not for Windows

Fix influxdata#3747: Fixing snmp tests on Windows by adding also equa…

8ba2bc3

…l comparision as it can happen very fast

vlastahajek added 5 commits September 5, 2019 16:06

Fix influxdata#3747: Fixing socker_listener tests on Windows by closi…

85cf1f3

…ng pre-created sock file and skipping unixgram test as unixgram is not support on Windows

Fix influxdata#3747: Skipping syslog tests on Windows as it is not fo…

149ca43

…r Windows

Fix influxdata#3747: Fixing x509 tests on Windows by converting also …

b3ee294

…Windows path to file URL

Fix influxdata#3747: Fixing x509 tests on Windows by converting to pa…

4ce197b

…th for file URL always to slash separated and skipping chmod on Windows

Fix build: Fixing build on older go versions

b05acec

Fix: Fixing typo

9eeca7c

danielnelson added the fix pr to fix corresponding bug label Sep 5, 2019

vlastahajek added 3 commits September 12, 2019 16:46

Fix influxdata#3747: Fixing exec on Windows by normalizing path to sl…

b5db72d

…ash forward separator

Fix influxdata#3747: Fixing exec tests on Windows: Normalizing input …

71ab96c

…path

Revert "Fix influxdata#3747: Fixing exec on Windows: Using different …

e126246

…command line split method for Windows" This reverts commit 046ed44

vlastahajek added 10 commits September 16, 2019 10:12

Fix influxdata#3747: Allowing all tests in short mode on Windows

5e1a1f7

Merge branch 'master' into vh-windows-tests-fixing

80fd675

Fix influxdata#3747: Skipping even unix socket test on Windows, as th…

e999e5e

…ey are not supported on older Windows (< Windows 10 build 17063)

Fix influxdata#3747: Skipping even unix socket test on Windows, as th…

2e0b07b

…ey are not supported on older Windows (< Windows 10 build 17063)

Fix influxdata#3747: Making err message comparision independent of OS…

005074f

… specific error message

Revert "Fix influxdata#3747: Fixing string comparison with line endin…

61e4065

…g to be platform agnostic" This reverts commit b5b5519

Revert "Fix influxdata#3747: Fixing filesystem_helper tests on Window…

f8cc3bd

…s by counting also CR char" This reverts commit 7da21e7

Fix influxdata#3747: Reverting fixes depending of line end character(s)

764678a

Fix influxdata#3747: Making sure testdata files had line ends lf

294bddc

Fix influxdata#3747: Making comparison line end character independent

bcb2396

zak-pawel mentioned this pull request Nov 15, 2020

Fix tests for Windows - part 1 #8414

Merged

3 tasks

sjwang90 added the platform/windows label Nov 18, 2020

sspaink closed this Mar 26, 2021

Conversation

vlastahajek commented Aug 14, 2019

Required for all PRs:

Uh oh!

vlastahajek commented Aug 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlastahajek Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlastahajek Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlastahajek commented Sep 5, 2019

Uh oh!

danielnelson commented Sep 13, 2019

Uh oh!

vlastahajek commented Sep 17, 2019

Uh oh!

sjwang90 commented Nov 18, 2020

Uh oh!

sspaink commented Mar 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vlastahajek Sep 6, 2019 •

edited

Loading

vlastahajek Sep 5, 2019 •

edited

Loading