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.

Sign up for GitHub

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

Jump to bottom

Adding known bugs to test cases #476

Closed
3 tasks done
Patrick-Poitras opened this issue Dec 29, 2021 · 4 comments
Closed
3 tasks done

Adding known bugs to test cases #476

Patrick-Poitras opened this issue Dec 29, 2021 · 4 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@Patrick-Poitras
Copy link

Patrick-Poitras commented Dec 29, 2021 •
edited
Loading

Proposal

This is a major change proposal regarding the addition of known issues into the testing suite for the purposes of detecting incidental fixes. This topic was mentioned on this discussion on Zulip that has been going around for the past few days.

In a nutshell, the proposal is to gather some known longstanding issues, and to run these tests at the end of the regular testing suite to notify users if their proposed submission fixes a known bug in the compiler. This would allow for incidental bugfixes to be noticed, which would allow for closing of issues that are incidentally fixed, and for the test cases to be added to regular tests to increase coverage. It would also ensure that temporary or draft code modifications that fix bugs incidentally are not discarded.

Roughly speaking, the changes that would need to happen are:

  • The addition of known bugs to the testing suite. (A rather large undertaking, though could be done by inexperienced editors, giving them some experience in contributing to rust)
  • The modification of the testing tools to allow for tests which are very likely to fail, and for a message to be displayed should this not be the case. (e.g. "Yay, you fixed an issue. Please submit it to us for consideration. (instructions)")

Mentors or Reviewers

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
@Patrick-Poitras Patrick-Poitras added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Dec 29, 2021
@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2021

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Dec 29, 2021
@jackh726
Copy link
Member

@rustbot second

I think this makes sense for sure. There's been good support on zulip so far.

Importantly, there isn't a ton of implementation work here. Really, this MCP is mostly for "policy" - if we want these kind of tests in the test suite, we should actively encourage people to contribute MCVEs from issues. However, there might be some implementation work to allow e.g. "relaxed" rules for error annotations or stderr capture. I'm willing to mentor/review these changes.

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Dec 29, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 30, 2021
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jan 20, 2022
@nikomatsakis
Copy link
Contributor

I'm a fan :) (I know I'm not supposed to comment here...)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 1, 2022
@jackh726 jackh726 mentioned this issue Feb 8, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 12, 2022
@matthiaskrgr
Rollup merge of rust-lang#93757 - jackh726:gat-bug-tests, r=nikomatsakis
59c76e2 
Add some known GAT bugs as tests

In the spirit of rust-lang/compiler-team#476

These tests are marked as "check-fail", but also commented with "this should pass". This many of the open GAT issues that are accepted bugs.

r? `@nikomatsakis`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 12, 2022
@matthiaskrgr
Rollup merge of rust-lang#93757 - jackh726:gat-bug-tests, r=nikomatsakis
8721866 
Add some known GAT bugs as tests

In the spirit of rust-lang/compiler-team#476

These tests are marked as "check-fail", but also commented with "this should pass". This many of the open GAT issues that are accepted bugs.

r? `@nikomatsakis`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 12, 2022
@matthiaskrgr
Rollup merge of rust-lang#93757 - jackh726:gat-bug-tests, r=nikomatsakis
36461e0 
Add some known GAT bugs as tests

In the spirit of rust-lang/compiler-team#476

These tests are marked as "check-fail", but also commented with "this should pass". This many of the open GAT issues that are accepted bugs.

r? ``@nikomatsakis``
@jackh726 jackh726 mentioned this issue Feb 12, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 16, 2022
@matthiaskrgr
Rollup merge of rust-lang#93953 - jackh726:known_bug, r=Mark-Simulacrum
cc7d11e 
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`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 18, 2022
@matthiaskrgr
Rollup merge of rust-lang#93953 - jackh726:known_bug, r=Mark-Simulacrum
620b0c5 
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``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nikomatsakis @apiraino @jackh726 @Patrick-Poitras @rustbot