Skip to content

[core] Add essential unit tests for TMap - #23703

Open
ravindra-RKB wants to merge 5 commits into
root-project:masterfrom
ravindra-RKB:feature-tmap-tests
Open

ravindra-RKB wants to merge 5 commits into
root-project:masterfrom
ravindra-RKB:feature-tmap-tests

Conversation

@ravindra-RKB

@ravindra-RKB ravindra-RKB commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

This Pull request:

Introduces essential unit tests for the TMap associative container in core/cont.

Changes or fixes:

  • Adds unit tests in core/cont/test/TMapTest.cxx covering:
    • Default initialization, capacity, and empty state checks
    • Key-value insertion and lookup by string key and object pointer (GetValue, operator(), FindObject)
    • Deliberate hash collision handling and bucket collision tracking
    • Entry removal via Remove (returning key) and RemoveEntry (returning TPair*)
    • Independent ownership semantics (SetOwner vs SetOwnerValue), removal cleanup, and DeleteEntry
    • Dynamic capacity expansion via Rehash
    • Lifecycle memory management across non-owner Clear(), DeleteKeys(), DeleteValues(), DeleteAll(), and owner Clear()
    • Forward traversal using TIter
  • Registers TMapTest in core/cont/test/CMakeLists.txt

Checklist:

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

@pcanal

pcanal commented Oct 13, 2026

Copy link
Copy Markdown
Member

Adds comprehensive test coverage in core/cont/test/TMapTest.cxx covering:

Is this PR meant to be comprehensive?

@pcanal

pcanal commented Oct 13, 2026

Copy link
Copy Markdown
Member

Dynamic capacity expansion and collision behavior via Rehash

The “collision behavior” test could be enhanced to deliberately create a collision; it currently only checks Collisions() for ordinary keys.

Also the ownership tests could cover both SetOwner() and SetOwnerValue() independently, or removal behavior when ownership is enabled.

@pcanal pcanal self-assigned this Oct 13, 2026
@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @pcanal!

  • Updated the PR description: these tests are focused on covering the essential and primary usage patterns of TMap.
  • Added a dedicated HashCollision test with a custom key class that forces identical hash values to explicitly verify bucket collision counts, lookup resolution under collision, and post-removal bucket counts.
  • Added an OwnershipIndependent test covering SetOwner() and SetOwnerValue() independently, verifying that value ownership triggers value cleanup on Remove(), and checking selective key/value deletion on Clear() and DeleteEntry().

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 564e1bb.

♻️ This comment has been updated with latest results.

@pcanal

pcanal commented Oct 14, 2026

Copy link
Copy Markdown
Member

Side note (if you did not already read it), check https://github.com/root-project/root/blob/master/CONTRIBUTING.md

@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Thanks for pointing that out @pcanal. I've re-read CONTRIBUTING.md, particularly the guidelines on AI disclosure and testing standards.

Regarding the previous test failure, my previous attempt tried forcing a small capacity (TMap(3, 0)) without realizing that THashTable enforces a minimum capacity of 17 (prime 19 via kInitHashTableCapacity), so the keys ended up in separate buckets.

I have updated the test with a helper class that returns a fixed hash value and overrides IsEqual(). This guarantees a bucket collision regardless of table size, and verifies that Collisions(), key lookup, and removal behave as expected under collision. All tests compile cleanly with /W4 and adhere to clang-format.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants