test: coverage for useCatalogSortIndex#71
Conversation
14 unit tests against the hook returned by useCatalogSortIndex(): - No-catalog fallback: empty map, hasSortIds=false, numeric-aware compare (AC-2 before AC-10) still works. - Catalog without sort-id props: same fallback path, but reached via the loaded-catalog branch. - Sort-id index population: top-level controls, groups themselves, control enhancements (controls.controls), nested groups (groups.groups), and the rare top-level catalog.controls case. Confirms non-sort-id props are skipped. - Comparator with index: ordering by sort-id, raw-input fallback when an operand is not indexed, case-insensitive lookup so pages passing lowercase IDs hit the catalog. - Memoization: stable instance across rerenders with the same catalog reference; re-derives when the catalog entry changes. - One BUG lock-in: the walker stores ids case-as-found in the catalog, but the comparator only tries toLowerCase() then raw — never toUpperCase(). So lowercase lookups miss when the catalog stored uppercase IDs. In practice OSCAL convention is lowercase so this rarely bites, but the asymmetry is documented here. The fix would be a one-line normalize in buildSortMap. Pure test additions. No executable source code changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| `toLowerCase()` then raw — never `toUpperCase()`. In practice OSCAL | ||
| convention is lowercase IDs so this rarely bites, but the asymmetry is | ||
| visible and would be a one-line fix (normalize keys to lowercase in | ||
| buildSortMap). Locked in here per the lock-in-before-fix discipline. */ |
There was a problem hiding this comment.
Locking in the behavior which has already shipped costs nothing and gains a baseline to push against when fixing. That is, fixing the bug requires updating the test to ensure correct behavior.
There was a problem hiding this comment.
Claude is pretty adamant about this one! But I haven't done the due diligence on it personally.
There was a problem hiding this comment.
The OSCAL standard expects string comparisons to be case sensitive per my understanding.
OSCAL leans on Metaschema, and this is the definition of string:
https://pages.nist.gov/metaschema/specification/datatypes/#string
Metschema in turn leans on the XML standard:
https://www.w3.org/TR/xmlschema11-2/#string
If the current comparison is case sensitive, then it should remain that way, otherwise it should be fixed.
There was a problem hiding this comment.
I did some reading on this, and as I understand it, the value associated with a sort-id could be upper, lower, or even mixed case. In other words, case sensitive. In this case, it might not be a bug per se, until it a match fails, then it would definitely be a bug.
My SOP for these situations is to "lock in" existing behavior with a passing test, then later follow with an update to the test which enforces correct implementation.
Thanks for your time!
2 participants
14 unit tests for the hook merged in #57. Pure test additions — no executable source code changes.
What's covered
hasSortIds: false, numeric-awarecomparesoAC-2still sorts beforeAC-10even without a catalog loaded.controls.controls)groups.groups)catalog.controls(rare but supported)sort-idprops are skippedOne BUG lock-in
The walker stores IDs case-as-found in the catalog, but the comparator only tries
.toLowerCase()then raw — never.toUpperCase(). So lowercase lookups miss the index when the catalog stored uppercase IDs. In practice OSCAL convention is lowercase IDs so this rarely bites, but the asymmetry is real. Documented in the test with a clearBUG:prefix and an explanatory comment; the fix would be a one-line normalize inbuildSortMap.Locking in the current behavior in this PR means a future fix is a clean assertion-flip in one place rather than a guess about what the comparator was supposed to do.
Setup
Tests use
OscalProviderwith a small seeding component (Seed) that callssetCatalogon mount viauseEffect. The fixture factories (ctrl(),group(),catalogOf()) keep the test bodies readable.Verification