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

Disable some flags on issue-71363 test #97750

Closed
wants to merge 1 commit into from

Conversation

JohnTitor
Copy link
Member

These flags make UI test fail on some env.
See #97682 for more details.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 5, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2022
// 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.
Copy link
Member

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)....

Copy link
Member Author

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added // ignore-test.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, does your PR fix the different behavior of this test described in #97682? If so, then we could close this and reopen #71363. This PR's motivation was to make UI test pass on @japaric's env and ignoring would be sufficient for that purpose.

Copy link
Member

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.
@pietroalbini
Copy link
Member

Opened #97789 with the proper fix for the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants