libtest: Fix unwrap panic on duplicate TestDesc #82274
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It is possible for different tests to collide to the same TestDesc when macros are involved. That is a bug, but it didn’t cause a panic until rust-lang#81367. For now, change the code to ignore this problem. Fixes rust-lang#81852. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
the
beta-nominated
|
Hm, are we just losing the result for the "other" test when things collide right now? It's a bit worrying that was missed; obviously an edge case but seems not great! I am going to accept this, as I think fixing the panic is likely the first thing we want (and particularly for a beta backport), but I would love to see a patch which avoids the collision somehow, and ideally adds asserts on any other maps/sets of TestDesc. Perhaps we should de-implement Hash on it and have some kind of wrapper type for maps... |
|
@bors r+ |
|
📌 Commit 1605af0 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 19, 2021
Rollup of 10 pull requests Successful merges: - rust-lang#79747 (Add explanations and suggestions to `irrefutable_let_patterns` lint) - rust-lang#81496 (name async generators something more human friendly in type error diagnostic) - rust-lang#81873 (Add Mutex::unlock) - rust-lang#82093 (Add tests for Atomic*::fetch_{min,max}) - rust-lang#82238 (ast: Keep expansion status for out-of-line module items) - rust-lang#82245 (Do not ICE when evaluating locals' types of invalid `yield`) - rust-lang#82259 (Fix popping singleton paths in when generating E0433) - rust-lang#82261 (rustdoc: Support argument files) - rust-lang#82274 (libtest: Fix unwrap panic on duplicate TestDesc) - rust-lang#82275 (Update cargo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
I agree, and I’ve now submitted that more complete solution as #82300. I don’t think we were losing the result from tests with duplicate |
|
This patch should still be backported. |
apiraino
added
the
T-compiler
apiraino
added
the
beta-accepted
Merged
cuviper
removed
the
beta-nominated
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 11, 2021
[beta] backports This backports some beta-accepted PRs and one additional LLVM fix for s390x. - rustdoc: treat edition 2021 as unstable rust-lang#82207 - Fix popping singleton paths in when generating E0433 rust-lang#82259 - libtest: Fix unwrap panic on duplicate TestDesc rust-lang#82274 - [intra-doc links] Don't check feature gates of items re-exported across crates rust-lang#82295 - rustdoc: Remove duplicate "List of all items" rust-lang#82484 - Substitute erased lifetimes on bad placeholder type rust-lang#82494 - Revert LLVM D81803 because it broke Windows 7 rust-lang#82605 - [SystemZ] Assign the full space for promoted and split outgoing args. rust-lang/llvm-project#95 r? `@Mark-Simulacrum`
bors
pushed a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 12, 2021
This more robustly avoids problems with duplicate TestDesc. See rust-lang#81852 and rust-lang#82274. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 12, 2021
libtest: Index tests by a unique TestId This more robustly avoids problems with duplicate `TestDesc`. See rust-lang#81852 and rust-lang#82274. Cc `@Mark-Simulacrum.`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is possible for different tests to collide to the same
TestDescwhen macros are involved. That is a bug, but it didn’t cause a panic until #81367. For now, change the code to ignore this problem.Fixes #81852.
This will need to be applied to
betatoo.