Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: normalize line endings in example output comparisons #51269

Closed
benhoyt opened this issue Feb 19, 2022 · 12 comments
Closed

testing: normalize line endings in example output comparisons #51269

benhoyt opened this issue Feb 19, 2022 · 12 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Proposal-Accepted
Milestone

Comments

@benhoyt
Copy link
Contributor

benhoyt commented Feb 19, 2022

If you have code which outputs \r\n (CR LF) as a newline delimiter on Windows, testable examples using this code will fail. I noticed this on my GoAWK project, which translates \n to \r\n on Windows to be a good Windows line-ending citizen on that platform. I'm currently using a !windows build constraint to skip this on Windows so these testable examples don't fail (source).

In addition, it's not obvious from the failure message what's going on, because even though it prints the got/want output, it looks correct (CR LF doesn't look any different from just LF when printed):

--- FAIL: Example (0.00s)
got:
foo
bar
want:
foo
bar
FAIL

Test output and source. My stripped-down example to show what's happening is as follows (though of course in GoAWK it's a bit more complex and hidden):

func Example() {
	if runtime.GOOS == "windows" {
		io.WriteString(os.Stdout, "foo\r\nbar\r\n")
	} else {
		io.WriteString(os.Stdout, "foo\nbar\n")
	}
	// Output:
	// foo
	// bar
}

It seems to me that examples should normalize line endings, and that these tests should succeed on Windows. When you're writing examples you don't want to be bothered by which kind of line ending the code is outputting. (If you care at that level, write a non-example test to ensure that behaviour.)

I'm on Go version 1.17, but this affects all versions going way back.

For reference, here are a few other "testable example whitespace" issues. Most of them are about spaces at the end of lines, so don't directly address this issue:

In #6416, Rob Pike says:

I would prefer to see the output of the examples use exact matching because that's
easiest to specify and allows one to test exact output. For the rare examples for which
that is problematic, sanitize/regularlize the output before matching. That is, write
code.

This could be taken as applying to this issue as well. I don't love this -- it is "easiest" to specify, but it seems to me not what you want, and definitely not visually obvious (which is what example tests are about). In any case, if that's the way it is, so be it. Maybe we could at least make the failure message more obvious in that case?

And if there are better ways to approach this problem in GoAWK or wherever else, I'm all ears!

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 20, 2022
@bcmills bcmills added this to the Backlog milestone Feb 22, 2022
@bcmills
Copy link
Contributor

bcmills commented Feb 22, 2022

#41980 could help somewhat here, in that the diff could at least (potentially) show the difference in line endings.

@bcmills
Copy link
Contributor

bcmills commented Feb 22, 2022

It seems to me that examples should normalize line endings, and that these tests should succeed on Windows. When you're writing examples you don't want to be bothered by which kind of line ending the code is outputting.

I tend to agree, but I think this is a subtle enough change to send through the proposal process. Do you want to go that route, or does #41980 suffice?

@benhoyt
Copy link
Contributor Author

benhoyt commented Feb 22, 2022

Thanks @bcmills. #41980 would definitely have helped me debug the issue to begin with (way back when). But I think it'd be even better if these tests could work on Windows, i.e., that the line endings were normalized. Would be great if this can be discussed as a proposal -- even if rejected.

@bcmills bcmills changed the title testing: examples that output \r\n on Windows fail (with a non-obvious message) proposal: testing: normalize line endings in output comparisons Feb 22, 2022
@bcmills bcmills modified the milestones: Backlog, Proposal Feb 22, 2022
@benhoyt
Copy link
Contributor Author

benhoyt commented Apr 3, 2024

I just re-ran this under Go 1.22 and the issue is still there (GitHub Actions failure).

It seems very reasonable to me to have code that outputs \r\n line endings on Windows, and also very reasonable that the testable examples take that into account, that is, normalize line endings (at least on Windows) before comparison.

Can this issue be added to the proposal review?

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

This seems completely reasonable to me. Thanks for the good use case description.

@rsc rsc changed the title proposal: testing: normalize line endings in output comparisons proposal: testing: normalize line endings in example output comparisons Apr 24, 2024
@rsc rsc added this to Proposals Apr 24, 2024
@rsc rsc moved this to Incoming in Proposals Apr 24, 2024
@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Apr 24, 2024
@rsc rsc moved this from Active to Likely Accept in Proposals May 8, 2024
@rsc
Copy link
Contributor

rsc commented May 8, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to convert example output \r\n into \n before comparing against the golden output in the source files, but only when running on Windows. This is necessary since source files have the same conversion applied to them, and some programs aim to print \r\n on Windows. On Unix, using \r\n is a mistake so we won’t do the conversion there.

(If we change our mind about Unix we can always change that later, but it seems right not to convert.)

@rsc
Copy link
Contributor

rsc commented May 15, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to convert example output \r\n into \n before comparing against the golden output in the source files, but only when running on Windows. This is necessary since source files have the same conversion applied to them, and some programs aim to print \r\n on Windows. On Unix, using \r\n is a mistake so we won’t do the conversion there.

(If we change our mind about Unix we can always change that later, but it seems right not to convert.)

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 15, 2024
@rsc rsc changed the title proposal: testing: normalize line endings in example output comparisons testing: normalize line endings in example output comparisons May 15, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 15, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/627035 mentions this issue: testing: replace CRLF by LF on windows before comparing to the expected output

@benhoyt
Copy link
Contributor Author

benhoyt commented Nov 13, 2024

Thanks @iwdgo for the fix!

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 19, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 19, 2024
@dmitshur
Copy link
Contributor

@ianlancetaylor Do you think this needs to be mentioned in Go 1.24 release notes? Or should this be considered a minor bug fix on Windows (despite going through the proposal process) and not worth mentioning?

@ianlancetaylor
Copy link
Member

I think this is a minor bug fix that doesn't need to be called out explicitly. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests

7 participants