Batch B (part 2): six components, and the test shapes that were hiding gaps - #157
Open
roncodes wants to merge 5 commits into
Open
Batch B (part 2): six components, and the test shapes that were hiding gaps#157roncodes wants to merge 5 commits into
roncodes wants to merge 5 commits into
Conversation
…decision
Eight tests, six exclusions, one defect recorded. Gaps 24 -> 3.
Tests:
- the sort comparator's final `?? 0` fallback, which only evaluates when the SECOND
channel it is handed has neither timestamp;
- a channel with no `participants` key at all, reaching the search's `?? []` fallbacks;
- a `chat.participant_removed` event, whose switch arm no test entered;
- a failed teammate load, which leaves an empty list instead of throwing;
- the notification sound playing for another participant's message and staying silent
for the user's own;
- the tray rendering its own declared defaults while channels and contacts are still
loading.
That last one matters more than it looks. `channels`, `unreadCount` and `availableUsers`
read as never-initialised, which is the same signature as the lazy `@tracked` defaults
excluded in Batch A. They are NOT that: the constructor assigns them inside an async
`withChannels` callback, and it only looks synchronous because every existing stub
resolves immediately. In production the template renders against the declared defaults
first. Excluding them would have recorded a test-harness artefact as a property of the
source, so they got a test instead.
Exclusions, each traced to the specific thing that makes it unreachable:
- `defaultNewChatName`'s 'Untitled Chat' and its `>1` branch — `createChat` is the only
caller and early-returns on an empty selection, the sole condition reaching them;
- `createChat`'s empty-selection guard — the Create button renders
`@disabled={{not @Cancreate}}`;
- two `store.push()` null checks — it always returns a record;
- `channels ?? []` — declared `[]`, only ever assigned arrays;
- `unlockAudio`'s catch — `notificationSound` is constructed unconditionally and none of
the three calls throw synchronously;
- `availableUsers`'s default — its only reader is compose-panel's `{{#each @users}}`,
behind `{{#if @isloading}}`; that gate closes exactly when the task assigns the value.
DEFECTS.md #6: `getUnreadCount` is a second, unwired implementation of the unread count.
The badge is not broken — `countUnread` already computes it from the loaded channels — but
the two differ, since the task fetches a server total that would also cover channels not
currently loaded. Needs a decision rather than a fix, and the three remaining gaps in this
file are exactly that task: dead code cannot be covered, and excluding it would hide the
question instead of answering it.
Full suite 5025 pass / 0 fail / 0 skip.
Coverage 94.22% statements, 90.18% branches, 97.44% functions, 94.65% lines.
Three tests, seven exclusions. The file now reports 0 uncovered statements and 0 partial branches. Tests: - an explicitly null @subjects. The destructured default only covers `undefined`, so a caller passing null reaches the `?? []` behind it; - a group that is a plain object rather than an Ember Data record, taking the `g.customFields = next` assignment instead of `g.set(...)`; - creating a field group BEFORE the subject has finished loading, when it genuinely has no `groups` key and the spread falls back to an empty list. Exclusions, each traced to the callers that make it unreachable: - the `subjects` initializer and its `?? []` — the constructor assigns synchronously before anything reads it; - `loadCustomFields`' subject guard — both callers check the subject first; - the three group-lookup guards in `#updateGroupOnSubject` — only reachable from a rendered group's own controls, so the subject always carries that group; - its object-patch branch — the method has exactly one caller and it always passes a function. Four silent failure modes cost a round each here and are worth recording, because none of them produce an error, a lint warning or a failing test — the only symptom is the gap still sitting in the report: 1. An `istanbul ignore` before `} else if (...)` attaches to the end of the preceding block. An if/else-if chain needs `ignore else` before the OPENING `if`. 2. The same shape before `} catch` rather than before the statement inside the catch. 3. A scripted replacement whose search string had the wrong indentation simply did not match, and said nothing. Every replacement now asserts. 4. A test can assert something true and still not reach the branch being claimed: clicking "New field group" creates the record, but `#appendGroupToSubject` only runs inside the modal's confirm callback after save. The branch counter showed it had run exactly once across 37 tests; the green assertion did not. Also fixed a selector that matched the wrong control: `buttonWithText('New field')` also matches "New field group", so the test was clicking create-group instead of create-field. It failed loudly only because its assertion checked the actual outcome. Full suite 5028 pass / 0 fail / 0 skip. Coverage 94.33% statements, 90.42% branches, 97.44% functions, 94.71% lines.
… never used
comment-thread/comment 14 gaps -> 1. Every gap was the same idea in two variants, and the
existing tests could reach neither because they all pass a fully-populated contextApi:
- the component's STANDALONE mode, where it talks to the store directly — delete via
destroyRecord, reply via store.createRecord + save + reload, edit via save;
- the validation-reject paths, where a contextApi reports the input invalid and the
action must do nothing and leave the form open.
Five tests across those two shapes rather than one per branch. This is the opposite of the
previous two files, where the remaining gaps were private defensive guards best settled by
counting callers; here they were a genuine second mode of the public component.
model-select 14 -> 8. Two tests for handler slots that are optional and were always
supplied: choosing a create-suggestion with no @onCreate, and clearing a selection so
`model?.id ?? null` reaches its fallback. Clearing turned out to be opt-in via @allowClear,
which the shared TEMPLATE does not pass — so that test renders its own.
Excluded both permission guards (old tracker #163): denying a permission also sets
`disabled` in the constructor, and power-select refuses to open a disabled trigger, so
neither task can run with the condition true. Asserted the pattern matched exactly twice
before replacing.
Deliberately NOT excluded: `getConfigOption`'s `return value`. It reads [340,0] only
because the dummy app defines no `ember-model-select` config, so every lookup returns
undefined and takes the default. That is a property of the harness, not of the code — real
consumers set that config. Excluding it would repeat the mistake caught in chat-tray, where
a stub's synchronous behaviour made three live `@tracked` defaults look dead. Left
uncovered with the reason recorded.
That distinction is now the main judgement call in this work: unreachable IN THE CODE versus
merely unreached BY THE HARNESS. Both look identical in the report. Getting it wrong in the
excluding direction is how a gate reaches 100% while hiding untested code.
Full suite 5035 pass / 0 fail / 0 skip.
Coverage 94.44% statements, 90.56% branches, 97.49% functions, 94.83% lines.
…tests stopped
metadata-editor 13 gaps -> 5, chat-window 15 -> 11.
Tests:
- a key cleared back to empty, reaching both the `|| ''` guard and `#toSnakeCase`'s
empty-input return;
- shift+enter keeping the draft instead of sending;
- a failed available-users load being reported rather than thrown;
- the existing self-removal test EXTENDED to confirm the dialog, which is where the
`isRemovingSelf` close actually happens.
That last one is the recurring shape in this batch, and worth naming: a test asserts the
confirmation appeared and stops there, while the branch under test lives in the confirm
callback. custom-fields-manager had the identical gap — clicking "New field group" creates
the record, but `#appendGroupToSubject` only runs after the modal's save. Extending the
existing test beats adding a second one: it covers the whole path from click to effect, and
it inherits setup that is easy to get wrong when rebuilt (my duplicate never found the
remove control at all).
Exclusions:
- MetadataRow's four @Tracked fields — the constructor assigns all of them;
- `coerceValue`'s `value == null` arm. Traced rather than assumed: null entries never
become rows, because `#isPrimitive` rejects them (typeof null is 'object'), the row
initialises `data.value || ''`, and every reassignment is a coerced value or an input
string. My first attempt was a TEST for this, which failed by seeding no row at all —
that failure is what established the exclusion is correct.
DEFECTS.md #7: metadata-editor's `label` getter is referenced by no template. Unlike the
other two dead getters this one has a visible consequence — the getter defaults the heading
to "Metadata", the template renders `{{@Label}}` directly, so a caller omitting the argument
gets NO heading rather than the default. An existing test already pins that behaviour.
Resolving it either starts rendering a heading everywhere a caller omits the label, or
deletes the getter; both are product calls.
Two wrong assumptions corrected while writing these: `loadAvailableUsers` reads
`store.query`, not `fetch.get` — stubbing the wrong service made the test fail on an empty
warnings array rather than on the behaviour; and `model-select`'s clear control is opt-in
via @allowClear, which the shared TEMPLATE does not pass.
Full suite 5038 pass / 0 fail / 0 skip.
Coverage 94.49% statements, 90.61% branches, 97.49% functions, 94.88% lines.
`removeButtonFor` was added alongside a duplicated self-removal test. That test was dropped in favour of extending the existing one, which left the helper unused and `pnpm run lint` exiting 1 on this branch. It went unnoticed because my verification ran the linter through a pipe (`eslint --fix <files> | grep -v WARN | head -3`), and a pipe discards the exit status, so a real no-unused-vars error read as success. `lint:js` also uses `--cache`, which can mask a regression until the cache is cleared. Verified here with `rm -f .eslintcache && pnpm run lint`.
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
1 participant
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.
Six of Batch B's largest files. Four commits, reviewable in order.
No production behaviour changes
Verified mechanically: the
addon/diff contains no deletions and no non-comment additions.Results
custom-fields-managercomment-thread/commentchat-traymetadata-editormodel-selectchat-window5038 pass / 0 fail / 0 skip. Both linters clean.
The finding worth your time: a test shape that hides gaps
Twice, independently, the same pattern: a test asserts a confirmation dialog appeared, then stops — while the branch under test lives in the confirm callback.
custom-fields-manager: clicking "New field group" creates the record, but#appendGroupToSubjectonly runs after the modal's save.chat-window: a test asserts the "leave this chat" copy, butisRemovingSelf → closeChannelonly fires on confirm.Both existing tests passed and looked thorough. The branch counter exposed them (
[1,0]— ran exactly once across 37 tests); the green assertions did not. Extending the existing test beat adding a new one — my from-scratch duplicate of thechat-windowcase never found the remove control, because the original had setup right that I rebuilt wrong.This generalises past coverage work: "never write an assertion that cannot fail" is not sufficient. An assertion can be perfectly falsifiable, test something genuinely true, and still be about a different line than the one you are crediting.
Unreachable in the code vs unreached by the harness
These look identical in the report and resolve oppositely. Three calls in this batch:
chat-tray's three@trackeddefaults — same signature as ~25 excluded in Batch A: close the small coverage gaps, record the unreachable ones #154, but live: the constructor assigns them in an async callback that only looks synchronous because every stub resolves immediately. Tested.model-select'sgetConfigOption— reads[340,0]only because the dummy app defines noember-model-selectconfig. Real consumers set it. Left uncovered, not excluded — with the reason recorded.custom-fields-manager's lookup guards — one caller, always a function. Excluded.Getting this wrong in the excluding direction is how a gate reaches 100% while concealing untested code.
Exclusion placement: four silent failure modes
None error, warn, or fail a test. The only symptom is the gap still in the report.
ignorebefore} else ifattaches to the preceding block — an if/else-if chain needsignore elsebefore the openingif.} catchrather than the statement inside it.table/thin Batch A: close the small coverage gaps, record the unreachable ones #154).New defect: #7
metadata-editor'slabelgetter is referenced by no template. Unlike #2 and #3 this one has a visible consequence: the getter defaults the heading to "Metadata", the template renders{{@label}}directly, so a caller omitting the argument gets no heading at all. An existing test already pins that behaviour.Resolving it either starts rendering a heading everywhere callers currently omit the label, or deletes the getter. Product call — not made here.
What still needs a decision
Five items in
DEFECTS.md, three of which are one question (dead getters with no template reference: #2useEllipsis, #3isBoolean, #7label). Plus #6getUnreadCount, and thechat-traysocket event-name inconsistency (the component useschat.participant_addedin one switch andchat.added_participantin another — needs someone who knows what the server emits).These are gate-blocking, not cosmetic. Dead code cannot be covered, and excluding it would hide the question rather than answer it.
chat-traysits at 3 gaps that no test can close.Two structural blockers remain beyond them: Batch C's harness question (~a third of all remaining coverage) and #4's
layout/sidebarnondeterminism, which can fail a 100% gate with no code change.