BTree: share panicky test code & test panic during clear, clone #81300

ssomers · 2021-01-23T14:45:36Z

Bases almost all tests of panic on the same, richer definition, and extends it to cloning to test panic during clone.

Mark-Simulacrum · 2021-01-24T21:50:31Z

library/alloc/src/collections/btree/testing/crash_test_dummy.rs

@@ -0,0 +1,67 @@
+#[doc(hidden)]
+#[macro_export]
+macro_rules! crash_test_dummy_def {


I think instead of a macro we can move CLONES and DROPS to Arc fields inside the CrashTestDummy struct and then just have a regular module.

I don't understand. How can such fields be initialized and point to something particular for each test case, without a macro? I could do it by explicitly passing an instance of some manager to every CrashTestDummy instance, just like in ord_chaos.rs, but then a simple reference is fine.

Note that all of these tests, including the ones I pilfered the code template from, are totally single threaded. They just use atomics to avoid scary unsafe blocks accessing globals.

A reference would also be fine.

ssomers · 2021-01-27T21:42:56Z

@rustbot label: +S-waiting-on-review -S-waiting-on-author

Mark-Simulacrum · 2021-01-28T23:33:54Z

library/alloc/src/collections/btree/testing/crash_test.rs

+
+#[derive(Debug)]
+pub struct Scene {
+ bits_per_id: usize,


Can we add some kind of comment explaining this struct?

In particular it's not clear what value should be passed for bits_per_id or what that means, but more broadly a top-level comment here describing usage would be helpful.

bors · 2021-02-05T17:47:00Z

☔ The latest upstream changes (presumably #81257) made this pull request unmergeable. Please resolve the merge conflicts.

ssomers · 2021-02-05T23:50:46Z

@rustbot label: +S-waiting-on-review -S-waiting-on-author

Mark-Simulacrum · 2021-02-08T22:46:18Z

library/alloc/src/collections/btree/testing/crash_test.rs

+ }
+
+ /// Returns how many times a dummy has been queried. If set up by `new`,
+ /// this is a hexadecimal composition of the count for each dummy id.


Can we add an example or documentation on what a hexadecimal composition means to the CrashTest struct? It sounds like what this is doing is splitting the u64 into 16 "sections", each 4 bits large, which enables us to track the number for each of those sections (up to 15, I guess). But I'm a bit confused as to why we need something so complicated over a vector of atomics, for example, or even a Mutex<Vec<u64>> - I don't think we care about performance here?

I don't see the complication, but I'm also familiar with binary coded decimal registers. Instead I think involving a mutex complicates matters. Involving a vector probably simplifies, but it is (another) dependency of the btree test code on a cousin in the crate - to some degree, testing the branch you're sitting on. Given the simple use case, I prefer to pull the counters apart in individual local variables and leave the mapping over to the compiler.

The way you comment "up to 15, I guess" suggest to me you didn't notice that I tried to document that same point in the CrashTest::new member, and neither of us noticed I incorrectly said it's limited to 16, so I'm strengthened in my belief there's already too much documentation here.

Anyway, I think the next iteration away from the original is substantially better.

ssomers · 2021-02-18T10:55:24Z

@rustbot label: +S-waiting-on-review -S-waiting-on-author

Mark-Simulacrum · 2021-02-20T20:40:01Z

Agreed that this looks much better, thanks. @bors r+ rollup

bors · 2021-02-20T20:40:02Z

📌 Commit 3e1d602 has been approved by Mark-Simulacrum

…=Mark-Simulacrum BTree: share panicky test code & test panic during clear, clone Bases almost all tests of panic on the same, richer definition, and extends it to cloning to test panic during clone. r? `@Mark-Simulacrum`

…=Mark-Simulacrum BTree: share panicky test code & test panic during clear, clone Bases almost all tests of panic on the same, richer definition, and extends it to cloning to test panic during clone. r? ``@Mark-Simulacrum``

Rollup of 11 pull requests Successful merges: - rust-lang#81300 (BTree: share panicky test code & test panic during clear, clone) - rust-lang#81706 (Document BinaryHeap unsafe functions) - rust-lang#81833 (parallelize x.py test tidy) - rust-lang#81966 (Add new `rustc` target for Arm64 machines that can target the iphonesimulator) - rust-lang#82154 (Update RELEASES.md 1.50 to include methods stabilized in rust-lang#79342) - rust-lang#82177 (Do not delete bootstrap.exe on Windows during clean) - rust-lang#82181 (Add check for ES5 in CI) - rust-lang#82229 (Add [A-diagnostics] bug report template) - rust-lang#82233 (try-back-block-type test: Use TryFromSliceError for From test) - rust-lang#82302 (Remove unsafe impl Send for CompletedTest & TestResult) - rust-lang#82349 (test: Print test name only once on timeout) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned Mark-Simulacrum Jan 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2021

ssomers force-pushed the btree_cleanup_leak_tests branch from 619493c to 64362d0 Compare January 23, 2021 17:00

ssomers changed the title ~~BTreeMap/BTreeSet: share more test code~~ BTreeMap/BTreeSet: test panic during clone & share panicky test code Jan 23, 2021

Mark-Simulacrum reviewed Jan 24, 2021

View reviewed changes

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 Jan 24, 2021

ssomers force-pushed the btree_cleanup_leak_tests branch from 64362d0 to 058b8f3 Compare January 25, 2021 14:15

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2021

ssomers force-pushed the btree_cleanup_leak_tests branch from 058b8f3 to 10e0f1c Compare January 28, 2021 17:23

Mark-Simulacrum reviewed Jan 28, 2021

View reviewed changes

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 Jan 28, 2021

ssomers force-pushed the btree_cleanup_leak_tests branch 2 times, most recently from 991bf7d to 440d869 Compare January 31, 2021 15:19

ssomers mentioned this pull request Feb 5, 2021

BTreeMap: clean up implementation of clone #81276

Closed

ssomers force-pushed the btree_cleanup_leak_tests branch from 440d869 to b54172b Compare February 5, 2021 23:31

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 5, 2021

ssomers changed the title ~~BTreeMap/BTreeSet: test panic during clone & share panicky test code~~ BTree: share panicky test code & test panic during clear, clone Feb 5, 2021

Mark-Simulacrum reviewed Feb 8, 2021

View reviewed changes

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 8, 2021

ssomers added 2 commits February 9, 2021 11:21

BTreeMap/BTreeSet: separate off code supporting tests

6d2247e

BTreeMap: share panicky test code & test panic during clear, clone

3e1d602

ssomers force-pushed the btree_cleanup_leak_tests branch from b54172b to 3e1d602 Compare February 9, 2021 12:05

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2021

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021

JohnTitor mentioned this pull request Feb 21, 2021

Rollup of 12 pull requests #82352

Closed

JohnTitor mentioned this pull request Feb 21, 2021

Rollup of 11 pull requests #82355

Closed

JohnTitor mentioned this pull request Feb 21, 2021

Rollup of 11 pull requests #82359

Merged

bors merged commit 3219a10 into rust-lang:master Feb 21, 2021

rustbot added this to the 1.52.0 milestone Feb 21, 2021

ssomers deleted the btree_cleanup_leak_tests branch February 21, 2021 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BTree: share panicky test code & test panic during clear, clone #81300

BTree: share panicky test code & test panic during clear, clone #81300

ssomers commented Jan 23, 2021 •

edited

Loading

Mark-Simulacrum Jan 24, 2021

ssomers Jan 24, 2021

Mark-Simulacrum Jan 25, 2021

ssomers commented Jan 27, 2021

Mark-Simulacrum Jan 28, 2021

bors commented Feb 5, 2021

ssomers commented Feb 5, 2021

Mark-Simulacrum Feb 8, 2021

ssomers Feb 9, 2021

ssomers commented Feb 18, 2021

Mark-Simulacrum commented Feb 20, 2021

bors commented Feb 20, 2021

BTree: share panicky test code & test panic during clear, clone #81300

BTree: share panicky test code & test panic during clear, clone #81300

Conversation

ssomers commented Jan 23, 2021 • edited Loading

Mark-Simulacrum Jan 24, 2021

Choose a reason for hiding this comment

ssomers Jan 24, 2021

Choose a reason for hiding this comment

Mark-Simulacrum Jan 25, 2021

Choose a reason for hiding this comment

ssomers commented Jan 27, 2021

Mark-Simulacrum Jan 28, 2021

Choose a reason for hiding this comment

bors commented Feb 5, 2021

ssomers commented Feb 5, 2021

Mark-Simulacrum Feb 8, 2021

Choose a reason for hiding this comment

ssomers Feb 9, 2021

Choose a reason for hiding this comment

ssomers commented Feb 18, 2021

Mark-Simulacrum commented Feb 20, 2021

bors commented Feb 20, 2021

ssomers commented Jan 23, 2021 •

edited

Loading