containers: link unit tests to comp_req requirements via RecordProperty - #568
Nikhil2206 wants to merge 5 commits into
Conversation
|
Documentation preview for this pull request is available at: |
There was a problem hiding this comment.
🟡 Changes recommended
Several links contradict or incompletely cover their requirements, and multiple metadata classifications are incorrect.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds requirement-traceability metadata to score/containers C++ unit tests.
Changes:
- Links tests to container component requirements.
- Adds test type, derivation technique, and descriptions.
- Leaves intentionally non-verifying tests unlinked.
File summaries
| File | Description |
|---|---|
dynamic_array_test.cpp |
Adds DynamicArray traceability metadata. |
intrusive_list_test.cpp |
Links intrusive-list tests to requirements. |
non_relocatable_vector_test.cpp |
Annotates swap and pointer-interaction tests. |
non_relocatable_vector_emplace_back_test.cpp |
Annotates emplacement and allocation tests. |
non_relocatable_vector_special_member_funcs_test.cpp |
Annotates lifecycle and special-member tests. |
Review details
Suppressed comments (11)
score/containers/non_relocatable_vector_special_member_funcs_test.cpp:373
- This test explicitly verifies that copy construction performs a new allocation, which is the opposite of the linked
comp_req__containers__deterministic_behaviorstatement that the library performs no dynamic memory allocation (score/containers/docs/requirements/index.rst:92). Reconcile and version the requirement or link this test to a requirement whose normative text actually permits and specifies this allocation.
this->RecordProperty("PartiallyVerifies", "comp_req__containers__deterministic_behavior");
score/containers/dynamic_array_test.cpp:465
- Passing the first out-of-range index is boundary-value testing of the API contract, not fault injection. Use the requirements-based test type so the generated verification metadata describes what this test actually does.
this->RecordProperty("TestType", "fault-injection");
score/containers/dynamic_array_test.cpp:751
- Calling
at(0)on an empty array is a plain boundary/out-of-range argument, which the traceability definition explicitly excludes fromfault-injection. Classify this as requirements-based.
this->RecordProperty("TestType", "fault-injection");
score/containers/dynamic_array_test.cpp:768
- Indexing an empty array is boundary-value testing of a contract violation, not an injected fault. Use the requirements-based test type required for this kind of off-spec API input.
this->RecordProperty("TestType", "fault-injection");
score/containers/dynamic_array_test.cpp:840
- This test supplies a valid zero-size input and checks empty-state/leak behavior; it does not inject a fault. Since its stated objective is memory leakage/resource consumption, classify it as
resource-usagerather thanfault-injection.
this->RecordProperty("TestType", "fault-injection");
score/containers/dynamic_array_test.cpp:866
- This valid zero-size copy exercises empty-state memory behavior without injecting any fault. The stated leak objective makes
resource-usagethe applicable test type.
this->RecordProperty("TestType", "fault-injection");
score/containers/non_relocatable_vector_emplace_back_test.cpp:211
- Exceeding reserved capacity by one is a boundary/off-spec API input, not fault injection under the mandated metadata definitions. Classify this contract check as requirements-based.
this->RecordProperty("TestType", "fault-injection");
score/containers/intrusive_list_test.cpp:206
- This test is built around a single-element list, which is classified as a collection boundary rather than a representative equivalence class. Record
boundary-valuesfor accurate generated verification evidence.
RecordProperty("DerivationTechnique", "equivalence-classes");
score/containers/intrusive_list_test.cpp:402
- The exercised list contains exactly one element, an explicit collection boundary in the derivation rules. This should be recorded as
boundary-values.
RecordProperty("DerivationTechnique", "equivalence-classes");
score/containers/intrusive_list_test.cpp:457
- A two-element collection is explicitly categorized as boundary-value derivation, not an equivalence-class representative. Update the metadata accordingly.
RecordProperty("DerivationTechnique", "equivalence-classes");
score/containers/intrusive_list_test.cpp:664
- This test deliberately covers single-element, empty-range, and full-range erase boundaries, so
boundary-valuestakes precedence overequivalence-classesunder the metadata rules.
RecordProperty("DerivationTechnique", "equivalence-classes");
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Several traceability records are incomplete or unsupported, and the modified architecture need requires a version bump.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
score/containers/non_relocatable_vector_special_member_funcs_test.cpp:376
- This touched test records the descriptive metadata but has no
FullyVerifiesorPartiallyVerifiesproperty, leaving an incomplete traceability record. Since the deterministic-behavior requirement cannot describe an operation that deliberately allocates, either remove the test metadata and document the deliberate exception or add an accurate requirement for this behavior.
this->RecordProperty("TestType", "resource-usage");
this->RecordProperty("DerivationTechnique", "equivalence-classes");
this->RecordProperty("Description",
"Check that copy-constructing a NonRelocatableVector allocates memory based on the "
"source's capacity, not its current size.");
score/containers/dynamic_array_test.cpp:821
- A leak check proves that allocated memory is released, not that no dynamic allocation occurred as required by
comp_req__containers__deterministic_behavior. With onlyEXPECT_TRUE(true)in the ordinary test configuration, this linkage records verification without an assertion of the requirement.
this->RecordProperty("PartiallyVerifies", "comp_req__containers__deterministic_behavior");
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Balanced
| :status: valid | ||
| :version: 1 | ||
| :fulfils: comp_req__containers__dynamic_array[version==1], comp_req__containers__intrusive_list[version==1], comp_req__containers__type_safety[version==1], comp_req__containers__non_relocatable_vector[version==1], comp_req__containers__deterministic_behavior[version==1] | ||
| :fulfils: comp_req__containers__dynamic_array[version==1], comp_req__containers__intrusive_list[version==1], comp_req__containers__non_relocatable_vector[version==1], comp_req__containers__deterministic_behavior[version==1] |
| TEST(EmptyDynamicArrayOfNonTrivialElementTypeMemoryTest, | ||
| TestNeverFailsButMemcheckDoesIfEmptyArrayIsNotCleanedUpCorrectly) | ||
| { | ||
| this->RecordProperty("PartiallyVerifies", "comp_req__containers__deterministic_behavior"); |
There was a problem hiding this comment.
I'm not sure we can do it better then what is done in the test. I'm ok with having it linked to the comp_req__containers__deterministic_behavior
| this->RecordProperty("TestType", "resource-usage"); | ||
| this->RecordProperty("DerivationTechnique", "equivalence-classes"); | ||
| this->RecordProperty("Description", | ||
| "Check that constructing a NonRelocatableVector with a non-zero element count allocates " | ||
| "exactly capacity() * sizeof(element) bytes up front."); |
There was a problem hiding this comment.
I would simply link it to comp_req__containers__non_relocatable_vector
| this->RecordProperty("Description", | ||
| "Check that move-assignment of non-trivial elements transfers values and destroys only the " | ||
| "previously held elements exactly once."); |
| TYPED_TEST(NonRelocatableVectorFixture, CallingEmplaceBackMoreTimesThanWereReservedTerminates) | ||
| { | ||
| this->RecordProperty("PartiallyVerifies", "comp_req__containers__non_relocatable_vector"); | ||
| this->RecordProperty("TestType", "fault-injection"); |
Summary
Part of #522. Adds
RecordPropertytraceability metadata linkingscore/containersunit tests to theircomp_req__containers__*requirements.Files touched:
dynamic_array_test.cpp,intrusive_list_test.cpp,non_relocatable_vector_test.cpp,non_relocatable_vector_emplace_back_test.cpp,non_relocatable_vector_special_member_funcs_test.cpp.Validated:
bazel test --config=bl-x86_64-linux //score/containers/...→ 7/7 test targets PASSED.Deliberately left unlinked (with reasoning)
non_relocatable_vector_special_member_funcs_test.cpp:CannotCopyVectorContainingNonCopyableType, and the twoNonMoveableAndCopyableElementTypeFixturevariants ofMoveConstructingMovesAllElements/MoveAssigningMovesAllElements— these have empty bodies (just comments documenting that type traits can't detect true non-compilability at runtime); nothing to assert or link.test/fake_memory_resource_test.cpp,test/fancy_pointer_allocator_test.cpp— these test-infrastructure self-tests (test doubles), notcontainerslibrary behavior. Nocomp_reqclaims anything about them, so left entirely unlinked.Fixes #523