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

BTreeMap: check some invariants in unit tests #75480

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Aug 13, 2020

No description provided.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2020
@Mark-Simulacrum
Copy link
Member

I think at this time I would avoid publicly exposing any introspection functions -- that shouldn't be needed for unit tests. If there's some reason we can't avoid it we can mark them unstable and #[doc(hidden)].

@ssomers
Copy link
Contributor Author

ssomers commented Aug 14, 2020

that shouldn't be needed for unit tests

I sympathise with that principle, but also with the viewpoint that unit tests shouldn't have more privilege than client code. Either way is fine for me. You tell me how to test things thoroughly.

  • With debug_assert!. Advantage is that you test use cases and values not covered in unit tests, disadvantage an overhead whenever you test something completely different. That new splitpoint is a poor example: the overhead is tiny and constant, but it is dead easy to cover the entire space of the function once up front in a "true" unit test (once you know how to write one).
  • With "true" unit tests with access privilege, such as the one for DormantMutRef. They are fine when something is quite stable, but terrible for an edit-test cycle, because x.py/cargo is not refined enough to see they are not part of the library.
  • With unit tests as they exist for btree code, without access privilege , as in this PR (though I did not remember #[doc(hidden)]).
  • With genuine introspection like StdBTreeMapProvider.

@Mark-Simulacrum
Copy link
Member

I'm perfectly fine with unit tests having access to the internal - in fact, that seems like a good thing - but I don't want that access to leak to everyone using BTreeMap (i.e., the visibility shouldn't be more than pub(super) or so).

If you put unit tests in a separate file and use #[cfg(test)] they should behave well with x.py/Cargo.

@ssomers
Copy link
Contributor Author

ssomers commented Aug 14, 2020

My assessment of "true" unit tests was based on old experience, in hashmap code, right now I see it behaving just fine in btree. And on the fact that all so-called btree unit tests are modelled as integration tests, but looking at the history of Cargo.toml, that stems from a merge of collections into alloc.

So are you saying that I can move the btree unit tests from the top tests directory to their "native" location? All of them, or what reason would there be to segregate? Some cases test immutable behaviour and I did not add check_invariant there, but that doesn't strike me as a good reason.

@Mark-Simulacrum
Copy link
Member

It should be fine to do so, yes - I'm not sure why they're split today. I don't care if we move all or just add new ones to the new location.

@ssomers ssomers marked this pull request as ready for review August 14, 2020 19:35
@ssomers ssomers changed the title Introduce BTreeMap introspection, check some invariants BTreeMap: check some invariants in unit tests Aug 15, 2020
@ssomers ssomers marked this pull request as draft August 15, 2020 11:21
@ssomers
Copy link
Contributor Author

ssomers commented Aug 15, 2020

Hold on, in times of social distancing it did not trickle down how close unit tests can be... no need to touch map.rs

@ssomers ssomers marked this pull request as ready for review August 15, 2020 11:56
@Mark-Simulacrum
Copy link
Member

I think we can probably just use panic! rather than returning a result with the invariant checking, seems cleaner (and easier to track down exactly where it failed).

@ssomers
Copy link
Contributor Author

ssomers commented Aug 17, 2020

I think we can probably just use panic!

I started that way. It's a little easier to write, but I found it harder to locate where in a test case the violation occurred. And it would allow to collect all violations, but being able to print the tree entirely trumps that. So back to panic.

Something that bugged me more is that there already is a function calculating length, and moreover it descends recursively, which in other places (in main code) is avoided. So an attempt at reusing this kind of tree navigation.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 18, 2020

📌 Commit 8c4641b 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 18, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
…rk-Simulacrum

BTreeMap: check some invariants in unit tests
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
…rk-Simulacrum

BTreeMap: check some invariants in unit tests
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…rk-Simulacrum

BTreeMap: check some invariants in unit tests
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…rk-Simulacrum

BTreeMap: check some invariants in unit tests
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…rk-Simulacrum

BTreeMap: check some invariants in unit tests
@bors
Copy link
Contributor

bors commented Aug 19, 2020

⌛ Testing commit 8c4641b with merge 443e177...

@bors
Copy link
Contributor

bors commented Aug 19, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 443e177 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2020
@bors bors merged commit 443e177 into rust-lang:master Aug 19, 2020
@ssomers ssomers deleted the btree_check_invariant branch August 19, 2020 17:27
@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
merged-by-bors This PR was explicitly merged by bors. 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