Skip to content

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

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

ravindra-RKB wants to merge 4 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

    24 files      24 suites   3d 23h 0m 56s ⏱️
 3 896 tests  3 896 ✅ 0 💤 0 ❌
83 416 runs  83 416 ✅ 0 💤 0 ❌

Results for commit 0e237cf.

♻️ 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.

@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Hi @pcanal, just following up on this. The branch is synced with latest master, and the collision and ownership tests are in place and passing cleanly across platforms. Whenever you have some time, could you please take a look? Thanks!

@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Hi @pcanal - just a gentle ping on this. The branch is updated and all TMap unit tests are passing cleanly in CI. Could this be merged when you have a moment? Thank you!

@pcanal pcanal closed this Oct 22, 2026
@pcanal pcanal reopened this Oct 22, 2026
@pcanal pcanal closed this Oct 22, 2026
@pcanal pcanal reopened this Oct 22, 2026
@pcanal pcanal closed this Oct 22, 2026
@pcanal pcanal reopened this Oct 22, 2026
@pcanal

pcanal commented Oct 22, 2026

Copy link
Copy Markdown
Member

This PR needs to be rebased. I tried to help out with the rebase but it appears that this branch is not set to allow PR maintainer to push to it.

@pcanal
pcanal force-pushed the feature-tmap-tests branch from 651cc16 to 0e237cf Compare October 22, 2026 22:33
@ravindra-RKB

Copy link
Copy Markdown
Contributor Author

Hi @pcanal - thank you so much for helping with the rebase! All 29 CI checks are green now. Could this be merged when you have a moment?

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