Repository navigation
Remove the jest cases the browser suite already asserts - #2239
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2239 +/- ##
==========================================
- Coverage 74.20% 74.09% -0.11%
==========================================
Files 129 129
Lines 3745 3745
Branches 829 865 +36
==========================================
- Hits 2779 2775 -4
- Misses 960 964 +4
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
size-limit report 📦
|
|
the replacement holds everywhere except score visibility, so this is the one thing to settle before it goes in.
either keep those three cases, or add a visible-score assertion at zero, positive and negative in the browser cases. #2240 needs a rebase anyway now that master moved, so it can carry the fix if that is easier. everything else checks out. I read each removed group against the browser case named for it: the vote buttons and the disabled upvote against |
The e2e suite drives the real widget and now reports what it reaches, so the cases it duplicates are two tests of one behaviour: one of them fails when the widget breaks and the other tells nobody anything new. The rule is narrower than coverage. A case goes only when a named e2e case asserts the same behaviour, and the unit of the decision is the assertion, not the case: a case that asserts a control renders and that its countdown reads 300s needs both covered, and the browser suite only waits for that element to appear. Coverage rules candidates out, never in, since a component renders during unrelated tests and marks its lines covered while nothing checks it. Twenty-four cases go, across nine suites. The largest share is comment.test.tsx, whose cases were almost all "does this element render", which a click proves: Playwright refuses to click a control that is missing, hidden or disabled, and renaming the Vote up title makes TestVote_UpvoteCountsOnce fail at its click. Thirty-three suites give up nothing. They assert redux actions, spy call counts, postMessage payloads, execCommand calls, CSS module class names, refs and icon dimensions, none of which the browser suite asserts and most of which it should not: asserting a request and its response is the better test of the same thing. Checked by measurement: with these cases gone jest covers no line it covered before that the browser suite does not also cover. That check caught one unsound deletion, the auth dropdown ignoring a clickOutside whose source is not window.parent, which no browser case reaches because the only such message the suite sends comes from the real parent and takes another branch. Line coverage cannot catch every kind of mistake, and two more came out of review. Hide rendering for an ordinary reader on another reader's comment was restored: TestThread_HideUserRemovesTheirCommentsOnly clicks Hide as the dev user, which the stack gives ADMIN_SHARED_ID, so it would pass unchanged if the control became admin-only. And a duplicate pair was deduped the wrong way round, leaving a case whose title says it asserts an empty form while its body asserts a restored draft; both are the behaviour the browser suite asserts, so both are gone. docs/backlog records what a browser-side change would release, grouped by the cause rather than by the file, since one fix frees cases across several suites.
The jest cases this branch removes asserted the counter's visibility at each of those values, and nothing in the browser suite did: the score is read through innerText, which returns the text of an element that is not rendered, so a display:none on the counter satisfied every browser assertion while the reader saw nothing. The three browser cases that already drive the counter through those values now assert it visible at each, beside the text they already read.
a67c883 to
8a25cd3
Compare
|
Added the browser assertions instead of keeping the three cases. The branch is rebased onto master now that #2228 and #2234 are in, so the diff is the one commit plus this one, and #2240 is rebased on top of it. Nothing else in the removal changed. |
Codecov fails the project status on any decrease. The frontend suite is shedding jest cases whose behaviour the browser suite asserts instead, and codecov measures only the jest half, so each such removal reads as a drop and paints the change red while covering nothing less. The threshold lets a deliberate removal pass and still fails a change that stops covering something.
|
One more commit since your pass: a |
The browser suite drives the real widget and #2234 reports what it reaches, so the jest cases it duplicates are two tests of one behaviour: one fails when the widget breaks, the other tells nobody anything new.
Twenty-four cases go, across nine suites. Thirty-three suites give up nothing.
Stacked on #2228 and #2234; the reviewable diff is the last commit alone.
The rule
A case may go only when a named e2e case asserts the same behaviour, and the unit of the decision is the assertion, not the case. A case asserting that a control renders and that its countdown reads
300sneeds both covered; the browser suite only waits for that element to appear, so that one was split and the countdown assertion kept.Coverage rules candidates out, never in. A component renders during unrelated browser tests and marks its lines covered while nothing checks it behaves correctly.
The commonest claim is that a browser case clicking a control covers a jest case asserting it renders. Playwright's
Clickrequires the element to exist, be visible, enabled and stable, and renaming theVote uptitle makesTestVote_UpvoteCountsOncefail at its click, atvote_test.go:49, rather than somewhere incidental downstream.What went, and from where
comment.test.tsxcomment-actions.spec.tsxcomment-votes.spec.tsxcomment-form.spec.tsxcreate-iframe.test.tsauth.spec.tsxcomment-form__subscribe-by-telegramprofile.spec.tsxsort-picker.spec.tsxcomment.test.tsxgave up the most because its cases were almost all "does this element render", which a click proves.Why thirty-three suites gave up nothing
They assert redux action objects, spy call counts,
postMessagepayloads,execCommandcalls, CSS-module class names, refs, icon dimensions and translation-tag fallbacks. The browser suite asserts none of those, and mostly should not: asserting a request and its response is the better test of the same thing.What the deletions were checked against
Jest coverage was generated with the deletions reverted and again with them applied, on this same tree, and the covered-line sets diffed against the widget profile from
make e2e-cover. Jest now covers no line it covered before that the browser suite does not also cover.That check is necessary and not sufficient, and three deletions got past it:
clickOutsidewhose source is notwindow.parent.auth.hooks.ts:30is entered by nothing else, because the only such message the browser suite sends comes from the real parent and takes another branchTestThread_HideUserRemovesTheirCommentsOnlyclicks Hide as the dev user, which the stack givesADMIN_SHARED_ID, so it would pass unchanged if the control became admin-onlyThe first came from the coverage diff; the other two from review, and neither could have been caught by it, since equal lines can carry different assertions. All three are resolved.
docs/backlog/jest-cases-the-browser-suite-could-take.mdrecords what a browser-side change would release, grouped by cause rather than by file, since one fix frees cases across several suites. Onlycomments-countergenuinely lacks a production selector;spinner,preloaderandcomment-actions-additionalalready carry static classes and simply have no browser case written yet.