test(edit): close two coverage gaps left by #682 (#817) - #927
Conversation
Deploying offlinecv with
|
| Latest commit: |
14ffb56
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8e3ea269.offlinecv.pages.dev |
| Branch Preview URL: | https://test-817-date-absence-and-fo.offlinecv.pages.dev |
Samhit21
left a comment
Samhit21
left a comment
There was a problem hiding this comment.
PR Review: test(edit): close two coverage gaps left by #682 (#817)
Summary
Both gaps are real and both fixes are verified the right way round — the mutation table is the part that makes this reviewable, and the "exactly 1 of 3 red, both controls green" row is the one that matters, because it shows the narrowing discriminates rather than just disabling the check. The two controls are well chosen: "still fires when the year survives in the date slot" and "keeps searching the wide slice for a non-date key" are precisely the two ways a narrowing like this goes wrong silently. No prior reviews on this PR, so everything below is first-pass.
The substantive finding is in the other direction from the PR's own framing: giving formatExperienceDateRange its first direct test has surfaced that the module docblock was already wrong about it. experience-dates.ts:82 says the function draws "" on the unanchored-is_current row where the two display formatters draw "Present". I ran all three — they all draw "Present". The actual divergence is one row over, on a whitespace-only start beside is_current: this function draws "Present", both siblings draw " –Present". That is a trim disagreement, it is reachable from the raw export path, and it is the intersection of the two behaviours this PR pins — currently covered by neither.
Findings
experience-dates.test.ts:273— the"Present"assertion is correct and contradictsexperience-dates.ts:82; correct the docblock here, while the test that falsifies it is being added. [Suggestion]experience-dates.test.ts:243— add{start_date: " ", is_current: true}. Verified"Present"here vs" –Present"in bothbuildProjectDatesandbuildDateRange; it is the only row where the formatters genuinely disagree onis_current, and the trim→unanchored-Presenttransition is otherwise unguarded. [Suggestion]corpus-edit-roundtrip.test.ts:399— the insertion point detachesexperienceFieldCheck's docblock ontoDATE_KEYS, orphaning the{@link experienceFieldCheck}at line 350. [Nit]corpus-edit-roundtrip.test.ts:421—editedRoleDateTextre-derives the marker lookupeditedRoleTextalready does,j()-ing every entry a second time per date check. Threadinghitin, or a sharedmarkedRole(experience), would also keep the two functions' fallbacks from drifting apart independently. [Nit]
Answering the two review-focus questions
Date slots rather than all four header fields — yes, and the reasoning is stronger than the one given. The argument in the PR body is the false-fail trade ("2019 Ventures"). But the narrowing forfeits nothing even in principle: replaced.start_date can only reach title/company if applyExperienceHeaderOverrides writes a date into a header field, which is a different defect class with its own dedicated gate, and the one way the collapse can misplace the value — leaving it in end_date — is inside the narrowed slice. It is also already covered from the other side by the end_date survived the lone-end-date collapse check at line 516. So this is not a trade; the wide slice was only ever buying false failures.
The fallback is fine, for a reason worth writing down. Every role's date slots is wider in role count but narrower in field count than editedRoleText's whole-experience JSON, so the two fallbacks are not the same "wide" despite the docblock citing the same justification. It does not matter here — the branch is only reached when the presence half has already failed, so the worst case is a second failure line, never a false green. But since they can now drift independently (finding 4), the docblock's "for the reason {@link editedRoleText} does" is worth a clause saying the forms deliberately differ and why it is safe that they do.
On fixing inside experienceFieldCheck rather than at the call site — agreed, and for the reason stated: the presence half is already field-aware, so a caller-side fix would put the field knowledge in two places. One cost to make visible: absenceText is now silently ignored for date keys, including at the corpus date call (line 506) and in two of the three new tests, which pass j(experience[0]) into a parameter that does nothing. The existing comment covers the why; a word at the call site that the argument is inert for this key would stop the next reader from trusting it.
Both gaps were green on main and both were one fixture away from mattering.
1. The corpus edit-leg absence check could be satisfied by role prose.
`experienceFieldCheck`'s absence half searched `absenceText` — for the date
call, `j(hit)`, the JSON of the whole edited role with `description` included —
for `replaced`, which is the fixture's OWN prior `start_date` and frequently a
bare "2019". Any future fixture whose role-0 prose merely mentions that year
("shipped the platform in 2019") would fail the gate for a reason unrelated to
the slot under test. The title/company keys are safe on that wide slice by luck,
not design: their literals are long and distinctive. A four-digit year is not.
Fixed inside `experienceFieldCheck` rather than at the call site. Its presence
half is already field-aware, so the absence half should be too, instead of every
caller having to remember which slice is safe for which key. A date key now
searches `editedRoleDateText` — the edited role's `start_date`/`end_date` slots
and nothing else.
The narrowing forfeits nothing: a replaced `start_date` can only reach
`title`/`company` if `applyExperienceHeaderOverrides` writes a date into a header
field, which is a different defect class with its own gate, and the one way the
collapse CAN misplace the value — leaving it in `end_date` — is inside the
narrowed slice and already asserted from the other side. The wide slice was only
ever buying false failures.
2. `formatExperienceDateRange` had no direct test.
It is exported and consumed on both the edit path (`ReconstructedRole`) and the
EXPORT path (`ats-resume-model.ts`), and its `.trim()` calls are a documented,
deliberate, reachable behaviour change on the latter. The #682 review found that
removing both trims left `src/lib/edit/**` and two repro suites entirely green.
Adds a `describe` block over the trim, the whitespace-only slot, the
`is_current` → "Present" substitution (including that it out-ranks a populated
`end_date`), the unanchored-`is_current` shape, the three fallthrough shapes and
the empty case.
3. The module docblock was wrong about the function, and nothing caught it.
Writing the test above falsified `experience-dates.ts`'s own prose: it claimed
the two display-only formatters draw "Present" where `formatExperienceDateRange`
draws "". All three draw "Present" — on any `is_current` row `end` is "Present"
before either fallthrough is reached, so "" is unreachable. The real divergence
is one row over and is a TRIM, not a collapse: on `{start_date: " ",
is_current: true}` this module draws "Present" while both siblings read " " as a
real anchor and draw " –Present". The docblock now says that, with the row
tested beside it, and the stale sibling line references are corrected.
That row is the intersection of the two behaviours this change pins — the trim
and the substitution were covered separately, never together — and it is the one
the module docblock names as the #672 hazard. Dropping both trims now turns 3
cases red rather than 2.
Both groups are falsified against the pre-fix code:
- Removing the two `.trim()` calls turns 3 of the 9 new cases red.
- Restoring the wide-slice absence half turns exactly 1 of the 3 new cases
red — the prose one — while the two controls stay green, so the new test
discriminates rather than passing trivially.
The only production change is comment-only: the corrected docblock. Full suite
6462 passed, up from 6450 by exactly the 12 cases added.
b674656 to
14ffb56
Compare
|
All four findings applied in Finding 4 — the duplicated marker lookup. Extracted On the fallback. Written into the docblock, in your terms: On On the narrowing. I took your reasoning over mine and put it in the docblock and the commit message: it is not a trade. A replaced One scope change you should know about. The PR is no longer strictly test-only: the docblock correction touches Gates: typecheck and lint clean, full suite 6462 passed | 8 skipped. Mutation table moved in the right direction — dropping both trims now turns 3 cases red rather than 2, which is the evidence that your suggested row was genuinely unguarded. Restoring the wide-slice absence half still turns exactly 1 of 3 red with both controls green. Branch collapsed back to a single commit. |
Samhit21
left a comment
Samhit21
left a comment
There was a problem hiding this comment.
PR Review: test(edit): close two coverage gaps left by #682 (#817)
Summary
All four findings applied, and I verified each rather than taking the replies at their word.
- The docblock correction is right. The replacement paragraph's table matches what the three functions actually return, and both stale sibling line references are now correct (
group-bullets.ts:366,entry-dates.ts:20). Keeping a note that the""claim was only ever true of the composite path is the right call — it stops the next reader re-deriving the thing that made the original wrong. - The new case earns its place by this PR's own measure. Dropping both trims now turns 3 cases red where it turned 2, which is the evidence the row was unguarded; the suite count moving 6461 → 6462 matches one added case exactly.
- The docblock is adjacent to
experienceFieldCheckagain and the{@link}at line 350 resolves. Catching that you had repeated the same detachment ontomarkedRolewhile fixing finding 4 — and that the inherited half was then outright false, sincemarkedRolereturnsundefinedrather than the whole-experience JSON — is the better catch of the two. Splitting into one docblock true of each is right. markedRoledoes what was asked, and picking upstringField's pre-existing detachment while in the file is fine by me — comment-only, same defect, same function cluster. Not worth its own PR.
The discussion points all landed too: the fallback asymmetry is written down, the narrowing reasoning is in the docblock and the commit message, and the inert absenceText is flagged at both the corpus call site and the two unit cases. On the scope change — a comment-only edit to experience-dates.ts is the correct place to fix a wrong comment in experience-dates.ts, and calling it out in the PR body is all that was needed.
CI is green on 14ffb56: verify, fallow, and Cloudflare Pages all pass.
Findings
Two comment-only nits, neither blocking, and one of them is mine:
experience-dates.ts:81— "exactly one row" undercounts the divergence; a trim disagreement necessarily shows up on every whitespace-only anchor, and I measured five such rows. [Nit]corpus-edit-roundtrip.test.ts:371—markedRole's docblock says the fallbacks "cannot drift apart independently" while line 418 says they "can drift independently". My finding-4 wording was the source of the imprecision: the shared lookup fixes the lookup, not the fallbacks. [Nit]
Both are one-line edits and neither changes behaviour, so I am not holding the PR for them — fold them into whatever you touch next in these files if you prefer.
Verdict
Action: APPROVE. The coverage this adds is real, the mutation evidence is what makes it reviewable, and the docblock it corrects along the way was a live falsehood about a function nothing had pinned.
| * formatters still hand-roll their own collapse and disagree with this one: | ||
| * `buildDateRange` (`score/group-bullets.ts:366`, which feeds | ||
| * `formatExperienceHeader`) and `buildProjectDates` (`score/entry-dates.ts:20`). | ||
| * The disagreement is a TRIM, not a collapse, and it shows up on exactly one |
There was a problem hiding this comment.
[Nit]: "exactly one row" undercounts it — the trim diverges on every whitespace-only anchor, not just the is_current one.
Ran the six rows against all three functions, normalising the siblings' tight dash so only the collapse is compared:
| input | this module | both siblings |
|---|---|---|
{is_current: true} |
"Present" |
"Present" — agree |
{start_date: " ", is_current: true} |
"Present" |
" –Present" |
{start_date: " ", end_date: "2022"} |
"2022" |
" –2022" |
{start_date: "2019", end_date: " "} |
"2019" |
"2019– " |
{start_date: " "} |
"" |
" " |
{end_date: " "} |
"" |
" " |
Five rows, not one. Which follows from the paragraph's own diagnosis: once the disagreement is a TRIM rather than a collapse, it has to show up wherever an anchor is whitespace-only, and is_current is irrelevant to it. The is_current row is the one that now has a test, which is not the same claim.
Nothing downstream changes — the safety argument ("reachable only from a raw export-path entry, never re-parsed") holds identically for all five. Suggest "on any row carrying a whitespace-only anchor" with the is_current row kept as the worked example, since that is the one #817 pinned.
| * synthetic marker, or `undefined` when none does. `NEW_TITLE`/`NEW_COMPANY` | ||
| * are literals that appear nowhere in any fixture, so at most one entry | ||
| * matches. Sole owner of the marker lookup — both slicers below derive from it | ||
| * so their fallbacks cannot drift apart independently (#817 review). */ |
There was a problem hiding this comment.
[Nit]: This says the fallbacks "cannot drift apart independently"; editedRoleDateText's docblock at line 418 says "the two can drift independently". My wording in finding 4 caused that — it was imprecise and you applied it faithfully.
What the extraction actually fixes is the lookup: one definition of "the marked role", so the two slicers cannot disagree about which entry they are slicing. The fallbacks are still written independently — fallback threaded in by the caller here, j(experience.map(slots)) there — and line 418 is right that they deliberately differ and that the difference is safe. Both things are true; it is only the sentence here that claims more than the change delivers.
Suggest scoping this one to the lookup and letting line 418 keep ownership of the fallback story:
* matches. Sole owner of the marker lookup — both slicers below derive from it,
* so they cannot disagree about WHICH entry they are slicing. Their no-marker
* fallbacks are separate and deliberately differ; see {@link editedRoleDateText}.
2 participants
Summary
Both gaps #817 identifies were green on
main, and both were one fixture away from mattering. Verified each still held before touching anything.Closes #817
Almost test-only. The single production change is comment-only: a docblock in
experience-dates.tsthat writing these tests proved wrong (see §3). No behaviour changes.1. The corpus edit-leg absence check could be satisfied by role prose
experienceFieldCheck's absence half searchedabsenceTextforreplaced. On the date call that slice isj(hit)— the JSON of the whole edited role,descriptionincluded — whilereplacedis the fixture's own priorstart_date, frequently a bare"2019". Any future fixture whose role-0 prose merely mentions that year ("shipped the platform in 2019", "since 2019") would fail the gate for a reason unrelated to the slot under test.The
title/companykeys are safe on that same wide slice by luck, not by design — their literals are long and distinctive. A four-digit year is not.Fixed inside
experienceFieldCheck, not at the call site. The issue offers either; I took the field-aware route because the function's presence half is already field-aware, so the absence half should be too, rather than every caller having to remember which slice is safe for which key. A date key now searcheseditedRoleDateText— the edited role'sstart_date/end_dateslots and nothing else.Scoped to the date slots rather than to all four header fields deliberately: a year can legitimately occur in a company name (
"2019 Ventures"), and the slots are where a failed collapse would actually leave the value. The no-marker fallback keepseditedRoleText's existing rule — fall back wide rather than silently skip the absence half, because the presence half is already failing in that case.2.
formatExperienceDateRangehad no direct testIt is exported and consumed on both the edit path (
ReconstructedRole) and the export path (ats-resume-model.ts'sexperienceDateRange), and its.trim()calls are a documented, deliberate, reachable behaviour change on the latter — an entry holdingstart_date: " "used to draw two spaces into the header slot and now draws nothing. The #682 review found that removing both trims leftsrc/lib/edit/**and two repro suites entirely green.Adds 8 cases: the trim, the whitespace-only slot, the
is_current→"Present"substitution (including that it out-ranks a populatedend_date), the unanchored-is_currentshape the docblock says reaches here only from the export path, the three fallthrough shapes, and the empty case.3. Writing the test falsified the module's own docblock
experience-dates.ts:82claimed the two display-only formatters draw"Present"whereformatExperienceDateRangedraws"". Reproduced before changing anything — all three draw"Present"; on anyis_currentrowendis"Present"before either fallthrough is reached, so""is unreachable for this function. The""claim was only ever true of the compositenormalizeExperienceDates→ format path thatReconstructedRoleruns.The real divergence is one row over and is a trim, not a collapse:
formatExperienceDateRangebuildProjectDatesbuildDateRange{is_current: true}"Present""Present""Present"{start_date: " ", is_current: true}"Present"" –Present"" –Present"That second row is the intersection this PR originally left uncovered — the trim and the substitution were each pinned, never together — and it is the row the docblock itself names as the #672 hazard. Both are now corrected and tested, and the stale sibling line references (
group-bullets.ts:328→:366,entry-dates.ts:16→:20) went with it.Falsified against the pre-fix code
This is the half of the AC that actually matters, so both groups were mutation-checked rather than argued:
.trim()callsThe second row is the useful signal: the new test discriminates rather than passing trivially, and the "still fires when the year survives in the date slot" and "keeps searching the wide slice for a non-date key" controls confirm the narrowing didn't just disable the check.
Why the #817 regression test lives at module scope, not in the corpus loop
The defect it pins is one no current fixture exhibits — it needs a role whose prose repeats its own start year. A corpus gate cannot self-test that class: it is green over the present fixtures and would go red on the next one added. So the three cases run over synthetic roles alongside the corpus suite. The docblock says so.
Review focus
src/lib/heuristics/corpus-edit-roundtrip.test.ts(editedRoleDateText) — narrowing to the date slots means a replaced date that leaked intotitleorcompanyis no longer caught. I judged that a worthwhile trade against the false-fail, since a year in a company name is realistic; do you agree, or would you rather it searched all four header fields?Test plan
npm run typecheckcleannpm run lintcleannpm run verifygreen withOFFLINECV_FULL_TESTS=1(also re-run by thepre-pushhook)main's 6450 by exactly the 12 cases addedfallowclean on both changed filesNo fixture binaries touched, so the fixture-PII preflight does not apply.