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

encoding/json: SyntaxError message change breaks brittle tests #42675

Open
dsnet opened this issue Nov 17, 2020 · 3 comments
Open

encoding/json: SyntaxError message change breaks brittle tests #42675

dsnet opened this issue Nov 17, 2020 · 3 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Nov 17, 2020

The change for #36221 adds a json: prefix to SyntaxError, which breaks >50 targets for the regression tests we run inside Google.

Thoughts:

  • Relying on the error message is to rely on unspecified behavior and is generally considered outside the Go 1 compatibility promise. The json package theoretically has the right to change error messages.
  • At the same time 50 targets is a non-trivial number of breakages. Fortunately they are all test failures (from my high-level glance).
  • If we're going to cause churn with a changed error message, should we also consider whether SyntaxError.Offset should be part of the error message string? Why or why not?
  • Should we batch changes like this for the future where we may want to introduce other changes to json that are technically compliant with the Go 1 compatibility promise, but may be slightly disruptive? Presumably it's better to break brittle targets once, rather than repeatedly.

Filing this issue to decide whether we're sure we want to move forward with this change.

\cc @mvdan @kevinburke @rsc

@dsnet dsnet added the NeedsDecision label Nov 17, 2020
@dsnet dsnet added this to the Go1.16 milestone Nov 17, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented Nov 17, 2020

Presumably it's better to break brittle targets once, rather than repeatedly.

I agree. Do we have more suggestions, though?

whether SyntaxError.Offset should be part of the error message string?

That could be a good idea, but how would you render it in the string? If you use ${offset}: ${message}, I think most people will incorrectly read the number as a line number and not a byte offset. We could go for ${message} at byte offset ${offset}, but it feels too wordy.

Another reason not to do it is that, right now, Error() is the only way to obtain just the message string. It might need to be coupled with exporting the msg string field.

At the same time 50 targets is a non-trivial number of breakages.

At this point I've learned to not try to argue with test breakages :)

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Nov 17, 2020

Yikes. I did try to search on Github for code snippets that this change could break, but that search wasn't exhaustive and probably Google writes better tests.

I think backing this change out is the right thing to do.

@neild
Copy link
Contributor

@neild neild commented Nov 18, 2020

To be clear, these tests are brittle and shouldn't depend on the exact text of stdlib errors. We're fine with dealing with it internally. However, 50 tests is more than we usually see break from a changed error message, which might point at this having more impact than expected in the ecosystem at large.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.