-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Disable some flags on issue-71363
test
#97750
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
// FIXME(#97682): | ||
// The `-Z simulate-remapped-rust-src-base=/rustc/xyz -Z ui-testing=no` flags | ||
// don't work well and make UI test fail on some env. | ||
// Once it starts to work fine, we could re-enable them here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test isn't disabled, right? I think we probably want to mark it as ignore for now, so that we can more easily tell that the test is not actually testing the right behavior and needs fixing.
Unless these flags are added for unrelated reasons? I don't see an obvious mention of them in the original issue (#71363)....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this PR disabled the flags, but didn't disable the test itself.
This test isn't disabled, right? I think we probably want to mark it as ignore for now, so that we can more easily tell that the test is not actually testing the right behavior and needs fixing.
Unless these flags are added for unrelated reasons? I don't see an obvious mention of them in the original issue (#71363)....
I just copy-pasted them from the previous PR so I cannot explain it correctly, I guess this comment has some context?: #71363 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added // ignore-test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the flag would indeed make the test incorrect.
This test ensures that when a file (in this case library/std/src/error.rs
) is not included in the error message it will not affect the indentation of the error message itself (due to the line number length).
The test tried to rely on -Z simulate-remapped-rust-src-base
to exclude the library/std/src/error.rs
file from the error message, but that flag is buggy. The flag simulated remap-debuginfo = true
only partially, and crucially didn't simulate mapping a remapped path to a local path when possible. Because of this, when remap-debuginfo = false
the file wasn't mapped to a local path (excluding it from the error), while when remap-debuginfo = true
it was mapped to a local file, causing the test to fail.
I opened #97786 to fix the behavior of that flag, but it also makes the test incorrect (as it always maps the remapped path to a local file).
If we'll want to fix this test we need to either find a path that will not be included in the error message regardless of remap-debuginfo
, or add a new -Z
flag to skip this piece of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm also working on a PR to try and add a -Z
flag to make the test work as expected.
These flags make UI test fail on some env. See rust-lang#97682 for more details.
Opened #97789 with the proper fix for the test. |
These flags make UI test fail on some env.
See #97682 for more details.