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

Migrate unit tests of btree collections to their native breeding ground #75531

Merged
merged 2 commits into from
Aug 15, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Aug 14, 2020

There's one BTreeSet test case that I couldn't easily convince to come along, maybe because it truly is an integration test. But leaving it in place would mean git wouldn't see the move so I also moved it to a new file.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2020
y.insert(2);
y.insert(1);

assert_eq!(hash(&x), hash(&y));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say more about why this didn't migrate? (E.g. the error log)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hash function is defined in the integration test itself and uses the hashmap crate or whatever contains it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really look into what DefaultHasher is, it looks foreign enough for me to say it's an integration test.

Copy link
Contributor Author

@ssomers ssomers Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather, to say it's not a btree unit test. In my book, it could be a hashmap unit test, built on top of btree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm okay, seems fine to leave it external, though I suspect we could internalize it without too much trouble if we really wanted to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see: it tests the default Hash implementation on BTreeSet. Why not test the non-default Hash implementation on BTreeMap for a change?

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 14, 2020

📌 Commit ff45df2 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Aug 14, 2020

Apparently I copied from the wrong pupil: everyone puts the mod tests early in the file, which makes sense. 3 bad boys put it at the bottom. Now there are 6.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2020
Rollup of 17 pull requests

Successful merges:

 - rust-lang#73943 (Document the unsafe keyword)
 - rust-lang#74062 (deny(unsafe_op_in_unsafe_fn) in libstd/ffi/c_str.rs)
 - rust-lang#74185 (Remove liballoc unneeded explicit link)
 - rust-lang#74192 (Improve documentation on process::Child.std* fields)
 - rust-lang#74409 (Change Debug impl of SocketAddr and IpAddr to match their Display output)
 - rust-lang#75195 (BTreeMap: purge innocent use of into_kv_mut)
 - rust-lang#75214 (Use intra-doc links in `mem::manually_drop` & `mem::maybe_uninit`)
 - rust-lang#75432 (Switch to intra-doc links in `std::process`)
 - rust-lang#75482 (Clean up E0752 explanation)
 - rust-lang#75501 (Move to intra doc links in std::ffi)
 - rust-lang#75509 (Tweak suggestion for `this` -> `self`)
 - rust-lang#75511 (Do not emit E0228 when it is implied by E0106)
 - rust-lang#75515 (Bump std's libc version to 0.2.74)
 - rust-lang#75517 (Promotion and const interning comments)
 - rust-lang#75519 (BTreeMap: refactor splitpoint and move testing over to unit test)
 - rust-lang#75530 (Switch to intra-doc links in os/raw/*.md)
 - rust-lang#75531 (Migrate unit tests of btree collections to their native breeding ground)

Failed merges:

r? @ghost
@bors bors merged commit 939befd into rust-lang:master Aug 15, 2020
@ssomers ssomers deleted the btree_tests_migration branch August 15, 2020 08:25
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 2, 2022
…crum

Classify BinaryHeap & LinkedList unit tests as such

All but one of these so-called integration test case are unit tests, just like btree's were (rust-lang#75531). In addition, reunite the unit tests of linked_list that were split off during rust-lang#23104 because they needed to remain unit tests (they were later moved to the separate file they are in during rust-lang#63207). The two sets could remain separate files, but I opted to merge them back together, more or less in the order they used to be, apart from one duplicate name `test_split_off` and one duplicate tiny function `list_from`.
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants