BTreeMap: check some invariants in unit tests

[ssomers]

No description provided.

[Mark-Simulacrum]

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]

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]

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]

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]

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]

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

[Mark-Simulacrum]

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]

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]

@bors r+

[bors]

📌 Commit 8c4641b has been approved by Mark-Simulacrum

[bors]

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

[bors]

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