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

pretty print source comparison failures are not fatal #52255

Closed
tinco opened this issue Jul 11, 2018 · 0 comments
Closed

pretty print source comparison failures are not fatal #52255

tinco opened this issue Jul 11, 2018 · 0 comments
Labels
C-bug Category: This is a bug. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@tinco
Copy link
Contributor

tinco commented Jul 11, 2018

Since 7948afd the compare_source check no longer panics on failure. It is obvious in the commit that the panic invocation is removed from the check, and no other changes are made in the function to remedy it.

It can be verified by adding a new pretty test case like:

// pp-exact
// The next line should not be expanded
mod issue_12590_b;

The resulting pretty printed file is slightly different, the comparison fails and the message is logged, but the test continues into the type check, which fails for a different reason and then panics. If you fix the type check (by adding the module and fn main) then the whole test passes and the error message is swallowed even though the source comparison is still not matching.

@Centril Centril added C-bug Category: This is a bug. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 11, 2018
bors added a commit that referenced this issue Jul 12, 2018
make pretty source comparison check be fatal (fixes #52255)

This is not ready for merging because it reveals (at least) two regressions in the pretty suite. Should I attempt to fix those in this PR also?
@bors bors closed this as completed in f0b1a78 Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants