[Core] MultiVecId tests - #6287
Open
alxbilger wants to merge 4 commits into
Open
Conversation
alxbilger
added
pr: status to review
Contributor
* [Core] Unittest: pin MultiVecId's conversion and assignment contract Most of TMultiVecId's API surface is a type system: which conversions are implicit, which must stay explicit, and which must not exist at all. None of that is reachable from a runtime EXPECT, so a refactor can silently drop an overload and every existing test still passes. Add static_asserts covering the three axes: access (write -> read only), vtype widening (specific -> V_ALL implicit, V_ALL -> specific explicit), and mutual exclusion of unrelated vtypes -- for construction and for assignment alike. * [Core] Unittest: check conversions carry and share the id map AccessConversionCompatibility only converts multi-vec ids whose map is empty, and only asserts on getDefaultId(). The converting constructor's whole reason for existing -- handing the per-state id map to the target without duplicating it -- was therefore untested. Cover both directions: same vtype across access, and specific vtype to V_ALL. Assert the contents and the storage identity; the no-copy part is the design contract documented on BaseVecId in VecId.h, and it sits on a path taken several times per solver iteration. * [Core] Unittest: cover MultiVecId assignment The suite had no assignment coverage at all -- every object was direct initialized, so operator= appeared nowhere in the file. Add two tests. The first pins the distinction between operator= and assign(): assigning a TVecId replaces the default id and keeps the per-state overrides, assign() drops them. MechanicalParams and ConstraintParams depend on that difference, using assign() for their TVecId setter overloads and operator= for the multi-vec ones. The second covers narrowing a V_ALL id to a specific vtype, which is explicit for construction but available as a plain assignment. * [Core] Unittest: cover the copy-on-write branch of writeIdMap Every setId() in the suite runs on an unshared map, so writeIdMap()'s clone-when-shared branch was never taken. That is the branch the shared_ptr::unique() replacement in this PR repairs -- on master the V_ALL half of this test does not even compile -- so it should not stay uncovered. Copy a multi-vec id that owns a map, write through the copy, and check the source is untouched and the storage has detached. Exercised on both the primary template and the V_ALL specialisation. * [Core] Unittest: assert the MultiVecId name format against literals operator<< is implemented as `out << v.getName()`, so asserting the stream against getName() compares the implementation to itself and would pass whatever getName() returned. Pin the format against literals instead. Use a single-state map so the result does not depend on the id map's iteration order, which is by BaseState address. Two shapes are covered: an entry whose vtype matches the default id, printed by index, and one whose vtype differs, printed by full name -- the latter branch of getName() had no coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unit tests mostly generated by Gemini 3.7 Flash.
I had to modify the MultiVecId class:
setId. Otherwise, an empty container would lead tohadIdMapreturningtrue, but with an empty map.setIdfor the classTMultiVecIdand its specialization forV_ALL: they both accept the same typestd::set<State>. I decided to remove the most generic overload (template<class StateSet> void setId(const StateSet& states, const MyVecId& id)), because it causes ambiguity (and compilation errors) when callingsetIdon aBaseState*(not aconst BaseState*).unique()has been removed in C++20 (https://en.cppreference.com/cpp/memory/shared_ptr/unique), which proves that this code was not even called anywhere. But the unit tests call it.[with-all-tests]
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if