-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Consolidate exhaustiveness-related tests #79243
Conversation
a8b4aa0
to
f51c94e
Compare
f51c94e
to
3213efc
Compare
match 0usize { | ||
0..10 => {}, | ||
10..20 => {}, | ||
5..15 => {}, // FIXME: should be unreachable |
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.
Huh, I'm surprised our tests didn't cover this before. Good catch (and below).
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.
We don't have an issue for this already, do we?
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.
We don't, but I've got a PR ready to fix it
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.
I don't know how to solve the other FIXME so I guess we could raise an issue. But it's quite tiny, I'm not sure it's worth it
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.
It's worth opening an issue, just so we can keep track of it.
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.
Done #79307
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.
It's always good to see more comprehensive tests (especially ones that catch some missing cases)! Thanks!
@bors r+ |
📌 Commit 3213efc has been approved by |
☀️ Test successful - checks-actions |
I hunted for tests that only exercised the match exhaustiveness algorithm and regrouped them. I also improved integer-range tests since I had found them lacking while hacking around.
The interest is mainly so that one can pass
--test-args patterns
and catch most relevant tests.r? @varkor
@rustbot modify labels: +A-exhaustiveness-checking