Skip to content

[core] Add essential unit tests for TBtree - #23824

Closed
ravindra-RKB wants to merge 1 commit into
root-project:masterfrom
ravindra-RKB:feature-tbtree-tests
Closed

ravindra-RKB wants to merge 1 commit into
root-project:masterfrom
ravindra-RKB:feature-tbtree-tests

Conversation

@ravindra-RKB

Copy link
Copy Markdown
Contributor

This Pull request:

Introduces essential unit tests for the TBtree container class in core/cont/test/TBtreeTest.cxx.

Changes or fixes:

Adds dedicated Google Test coverage for TBtree, ROOT's balanced B-tree collection inheriting from TSeqCollection:

  • DefaultConstructor: Verifies empty state, default order of 3, and non-owning state.
  • CustomOrder: Checks customized tree order and verifies warning diagnostic clamping when order < 3.
  • SortedInsertionAndLookup: Tests ordered insertion, index-based access (At, First, Last, operator[]), and key/pointer lookup (FindObject).
  • Rank: Verifies rank calculation of keys within the tree.
  • NodeSplittingAndLargeInsertion: Exercises leaf and inner node splitting, rebalancing under reverse-order insertion of 50 elements, and element removal across nodes.
  • RemoveAndClear: Tests individual element removal and tree clearing.
  • OwnershipAndClearDelete: Verifies object lifecycle management under non-owning and owning (SetOwner(kTRUE)) configurations for both Clear() and Delete().
  • Iteration: Tests forward and backward iteration with TIter and TBtreeIter.
  • ErrorDiagnosticsForNonSortable: Verifies error messages with ROOT::TestSupport::CheckDiagsRAII when non-sortable objects are passed to Add, FindObject, Rank, Remove, or when forbidden operations (Before, After) are called.
  • Registers TBtreeTest in core/cont/test/CMakeLists.txt.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@dpiparo

dpiparo commented Oct 25, 2026

Copy link
Copy Markdown
Member

You seem to invest effort in writing unit tests. Could you please give some context about the additional coverage this gives on top of the rest of the existing battery?

@dpiparo dpiparo self-assigned this Oct 25, 2026
@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Hi @dpiparo,

Existing tests for TBtree were mostly in legacy macros (test/tcollex.cxx) and iterator checks (test/stressIterators.cxx).

This PR adds isolated unit test coverage under core/cont/test focusing specifically on node splitting, rebalancing under sequential additions/removals, ranking, ownership semantics (Clear vs Delete), and diagnostics for non-sortable objects.

@guitargeek

Copy link
Copy Markdown
Contributor

Hi @ravindra-RKB,

thanks for the PR. The test itself is carefully written, but I am closing it, for the reason we already discussed in #23510 (comment): we add tests together with non-trivial changes to the code they cover, not retroactively for code that nobody is changing. A unit test earns its keep by catching a regression when the code under it changes, and this code has not changed in 17 years and has no reason to.

TBtree is the clearest case of this in your series of PRs. Nothing in ROOT uses it: outside its own source files, the only references are the legacy programs under test/, where tcollex and stressIterators already exercise it in CI. The class has had no functional change since 2008, and it is not persistent, so there is no I/O to protect either. A new test suite here would never fire on anything, while costing us another executable to build, run and maintain on every platform forever. If anything, TBtree is a candidate for a \legacy tag rather than for new tests.

I'd also ask you to stop opening further PRs in this series. Where a collection class is widely used and still evolves, like TList, the maintainers have taken the tests, but please leave the decision of where coverage is worth adding to them, and let's not go through the same discussion class by class. If you want to contribute to the core collections, fixing an actual bug or issue, with a test that fails without the fix, is what we are happy to review.

Thanks for understanding.

@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Thanks for the clear explanation @guitargeek, that makes total sense. I will focus on open issues and bug fixes going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants