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

Add the known-bug test directive, use it, and do some cleanup #93953

Merged
merged 4 commits into from
Feb 19, 2022

Conversation

jackh726
Copy link
Member

cc rust-lang/compiler-team#476

Now tests can be annotated with known-bug, which should indicate that the test should pass (or at least that the current output is a bug). Adding it relaxes the requirement to add error annotations to the test (though it is still allowed). In the future, this could be extended with further relaxations - with the goal to make adding these tests need minimal effort.

I've used this attribute for the GAT tests added in #93757.

Finally, I've also cleaned up header.rs in compiletest a bit, by extracting out a bit of common logic. I've also split out some of the directives into their own consts. This removes a lot of very similar functions from Config and makes TestProps::load_from read nicer.

I've split these into separate commits, so I in theory could split these into separate PRs if they're controversial, but I think they're pretty straightforward.

r? @Mark-Simulacrum

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 12, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2022
@Mark-Simulacrum
Copy link
Member

r=me with or without the nit applied, depending on how you feel about it.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2022
@jackh726
Copy link
Member Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Feb 14, 2022

📌 Commit 36cf48b has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 16, 2022
Add the `known-bug` test directive, use it, and do some cleanup

cc rust-lang/compiler-team#476

Now tests can be annotated with `known-bug`, which should indicate that the test *should* pass (or at least that the current output is a bug). Adding it relaxes the requirement to add error annotations to the test (though it is still allowed). In the future, this could be extended with further relaxations - with the goal to make adding these tests need minimal effort.

I've used this attribute for the GAT tests added in rust-lang#93757.

Finally, I've also cleaned up `header.rs` in compiletest a bit, by extracting out a bit of common logic. I've also split out some of the directives into their own consts. This removes a lot of very similar functions from `Config` and makes `TestProps::load_from` read nicer.

I've split these into separate commits, so I in theory could split these into separate PRs if they're controversial, but I think they're pretty straightforward.

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#89892 (Suggest `impl Trait` return type when incorrectly using a generic return type)
 - rust-lang#91675 (Add MemTagSanitizer Support)
 - rust-lang#92806 (Add more information to `impl Trait` error)
 - rust-lang#93497 (Pass `--test` flag through rustdoc to rustc so `#[test]` functions can be scraped)
 - rust-lang#93814 (mips64-openwrt-linux-musl: correct soft-foat)
 - rust-lang#93847 (kmc-solid: Use the filesystem thread-safety wrapper)
 - rust-lang#93877 (asm: Allow the use of r8-r14 as clobbers on Thumb1)
 - rust-lang#93892 (Only mark projection as ambiguous if GAT substs are constrained)
 - rust-lang#93915 (Implement --check-cfg option (RFC 3013), take 2)
 - rust-lang#93953 (Add the `known-bug` test directive, use it, and do some cleanup)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 620b0c5 into rust-lang:master Feb 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 19, 2022
@jackh726 jackh726 deleted the known_bug branch February 19, 2022 05:16
@petrochenkov
Copy link
Contributor

petrochenkov commented May 25, 2022

I didn't notice this PR soon enough, but this is a mistake, IMO.
//~ ERROR annotations make exactly the same sense and serve the same goals in "bug" tests as in any other tests - they are useful for human reader, and prevent human mistakes during test writing by providing redundancy.

@jackh726
Copy link
Member Author

@petrochenkov there was some discussion on zulip about this, but let me try to summarize what I can recall:

In regular tests, //~ ERROR can help by providing redundancy (I'm not fully convinced that they do). Importantly though, this redundancy is that the error message you expect matches the error message you get. In known-bug tests, we don't expect any error message. It doesn't make sense (at least to me) to add error annotations to a test that you expect to be different (compile, or fail with different error). Another reasonable argument, for example, would be that the error annotations are for the errors you expect; so if you expect the test to compile, you shouldn't see any error annotations.

On the contrary, keeping the stderr around does help, because it provides a "history" of why the test did not compile. If a test starts to pass, it helps you understand why and what changed.

@petrochenkov
Copy link
Contributor

In practice this makes tests hard to read and write for me, so I'm currently just asking people to avoid // known-bug if I encounter it during review (#97391 (comment)), and not using it myself.

@petrochenkov
Copy link
Contributor

In the linked issue I wanted to see where the bug is exactly, and what the bug is exactly, which is hard to do if the errors are in a different file and that file has lines anonymized, especially if the test gets larger than a few lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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

7 participants