golden: Add a var to disable normalization of crlf #149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
==========================================
- Coverage 83.91% 83.15% -0.77%
==========================================
Files 29 29
Lines 2058 2030 -28
==========================================
- Hits 1727 1688 -39
- Misses 225 234 +9
- Partials 106 108 +2
Continue to review full report at Codecov.
|
golden/golden.go
Outdated
| // you will need to set this to false. | ||
| // | ||
| // The default value may change in a future major release. | ||
| var NormalizeCRLFToLF = true |
There was a problem hiding this comment.
What about adding another flag like test.update-golden ? You can then control the CRLF from command line (or Makefile) depending you are on windows or not?
There was a problem hiding this comment.
Sorry for not responding for a year, I must have missed this comment and just noticed it now.
I don't think a flag is a good option in this case because you can't pass flags to the entire test suite. Any package that doesn't import golden will fail with "unexpected flag" error. So using a flag makes it impossible to run go test ./....
A flag is ok for the update-golden case because in general we expect to only update a single test , or package at a time. Having to run only 1 package to set the flag is fine in this case, maybe even desirable to prevent accidentally running it against the entire test suite.
I expect anyone setting NormalizeCRLFToLF would want it off for the entire package, so a global seems more correct. I just changed the default to read from an env var, which might be a better way of setting the value.
There was a problem hiding this comment.
Thanks that makes sense 👍
dnephin
force-pushed
the
golden-normalize
branch
from
9bb28b8
to
dc13988
Compare
There was a problem hiding this comment.
It's been a while since this PR was opened, so I'm going to restate my thinking here to see if it makes sense.
Desired outcome:
- all golden files use
\nline endings so that contributors on any platform can update them to the same value. - when the actual value from the unit-under-test contains
\r\nline endings we normalize them to\nso that they match the expected.
The original normalize feature solved this problem as long as git did not try to convert \n to \r\n on checkout.
When running on windows, and git has core.autocrlf enabled the golden files will be un-normalized on checkout and contain \r\n.
When core.autocrlf is enabled git has solved the normalization problem for us. It ensures that files are committed with \n line endings, but converts them to \r\n so that even though the actual values from a test contain \r\n, so does the expected golden file.
Had I known about this setting earlier, I think I would have suggested that we not add the normalize and rely on git to handle it. Now that we have it, I think we should add this option to disable the behaviour for repos which use the git option to handle it.
I think the normalization options we have in fs are correct because those files may not come from files in git, so git will not normalize those.
golden/golden.go
Outdated
| // you will need to set this to false. | ||
| // | ||
| // The default value may change in a future major release. | ||
| var NormalizeCRLFToLF = true |
There was a problem hiding this comment.
Sorry for not responding for a year, I must have missed this comment and just noticed it now.
I don't think a flag is a good option in this case because you can't pass flags to the entire test suite. Any package that doesn't import golden will fail with "unexpected flag" error. So using a flag makes it impossible to run go test ./....
A flag is ok for the update-golden case because in general we expect to only update a single test , or package at a time. Having to run only 1 package to set the flag is fine in this case, maybe even desirable to prevent accidentally running it against the entire test suite.
I expect anyone setting NormalizeCRLFToLF would want it off for the entire package, so a global seems more correct. I just changed the default to read from an env var, which might be a better way of setting the value.
dnephin
force-pushed
the
golden-normalize
branch
from
dc13988
to
f13b253
Compare
Possible fix for #146
I'm not sure if this is the best option yet, but from what I have read the
autocrlfconfig seems to default to true on windows. If this behaviour is likely to conflict with git defaults we should at least have a way to turn it off.