Repository navigation
test(frontend): fix the two tests #17365 names, and share the mechanism (#17365) - #17664
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change adds shared Vue test utilities for waiting on state and event emissions. It updates workflow canvas and contact-edit tests to wait for those conditions before making assertions. ChangesTest synchronisation
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The remaining issues affect test diagnostics and confidence in the wait-helper tests, not application behavior. The change is mergeable with these focused follow-ups. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 33d23362-da87-4e4a-808e-b8ffac6df4b0
📒 Files selected for processing (8)
autobot-frontend/src/components/workflow/__tests__/WorkflowCanvas.contextMenu.test.tsautobot-frontend/src/components/workflow/__tests__/WorkflowCanvas.marquee.test.tsautobot-frontend/src/components/workflow/__tests__/WorkflowCanvas.panClick.test.tsautobot-frontend/src/components/workflow/__tests__/WorkflowCanvas.undoRedo.test.tsautobot-frontend/src/components/workflow/__tests__/emitTestUtils.test.tsautobot-frontend/src/components/workflow/__tests__/emitTestUtils.tsautobot-frontend/src/test/utils/waitForState.tsautobot-frontend/src/views/llc/__tests__/OrgChart.contactEdit.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Non-author review of 88639a659f. The red is real, deterministic, and one line — it is not the #17365 nondeterminism. No test failed; the suite never got the chance.
What actually failed
frontend-tests failed in the Run frontend linting step, before any test ran:
> oxlint . -D correctness --ignore-path .gitignore --ignore-pattern 'src/types/_generated/**'
src/components/workflow/__tests__/WorkflowCanvas.contextMenu.test.ts:1:10:
Identifier 'waitForEmitCount' is imported but never used.
Found 0 warnings and 1 error.
ERROR: "lint:oxlint" exited with 1.
Confirmed at the head rather than from the log alone: line 1 is import { waitForEmitCount, waitForLastEmit } from './emitTestUtils', and waitForEmitCount occurs exactly once in the file — the import itself.
So: a correctness lint error from this PR's own migration. It will fail identically on every run, which is the opposite of the flake class — the one diagnosis that would have been costly here is "red carries no information, re-run it".
It is not #17365's nondeterminism, on three independent grounds
- No test executed.
run-s lint:oxlint lint:eslintfailed at the first step, so the vitest run never started. There is no failing-file set to intersect with the union of 10. - The shape does not match. #17365's signature is an assertion runs before the thing it asserts has settled — a timing failure inside a test. This is a static lint diagnostic at a fixed line.
- Only one check is red. Latest-per-check: 1 FAILURE (
frontend-tests), 34 SUCCESS, 10 SKIPPED, 1 NEUTRAL, 2 still running.Unit & Integration Testsis not failing at this head — so the "two coverage consumers, both red" reading that pointed at the flake does not hold either.
The fix
Delete waitForEmitCount from the import, or use it. Looking at the diff's intent — migrating contextMenu.test.ts onto the shared mechanism — the likely history is that the shared helper's other export is what this file needed, and the unused name came along.
Worth one glance while you are in there: the import sits above the copyright header (line 1, with the SPDX block at lines 2-3). Every other file in the tree carries the header first. That is consistent with an automated edit inserting at line 1, which is also the most likely reason an unused import survived review — nobody reads line 1 when the file appears to start at line 2.
On the rest of the PR
Nothing else in the run objects, and Refs #17365 rather than Closes is right for a partial against an issue the owner retitled to the class — the bar is does not make things worse, and a one-line lint slip does not clear or fail that bar, it just blocks the merge.
Base is red separately — the direct push at 66f071c17c fails the commit-convention gate — so ignore any convention failure you see here; it is not this PR's.
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Correcting my own count: two checks are red, not one. My selector was broken.
I wrote "only one check is red … Unit & Integration Tests is not failing at this head". That is wrong, and the way it went wrong is worth more than the correction.
Unit & Integration Tests has two instances at 88639a659f:
| job | conclusion | completed |
|---|---|---|
| 108695355351 | FAILURE | 19:56:15 |
| 108695326761 | SKIPPED | 19:54:38 |
My "latest-per-check" idiom was group_by(.name) | map(last) — which takes the last element in API order, not the newest by time. The API returns the FAILURE first here, so last selected the earlier, skipped instance and reported the check as not failing.
Corrected, sorting each group by completedAt:
failing: ["Unit & Integration Tests", "frontend-tests"]
tally: FAILURE=2 SUCCESS=35 SKIPPED=8 NEUTRAL=1 pending=2
One cause, two red checks — both workflows run the frontend lint step, so the single unused import fails both.
Nothing about the diagnosis changes. The cause is still waitForEmitCount imported and never used at line 1, still a correctness error at a fixed line, still deterministic, still not #17365's nondeterminism — the three grounds for that were no test executed, static diagnostic rather than an assertion racing a state change, and deterministic. None of them rested on the count. The fix is still the same one line.
What the count being wrong does change: I used it as an argument, and the argument was unsound. Two red coverage consumers is exactly the pattern that makes a flake hypothesis plausible, and I told the author it did not hold. It does hold as an observation; it is just not evidence of a flake, for reasons that have nothing to do with how many checks are red.
The selector is the real finding and it is not local to this PR — it is the idiom I have used for every check-status read today. map(last) after group_by silently picks by array position, and the direction of the error here is the dangerous one: it turned a FAILURE into a SKIPPED and made a red PR read partly green.
…wait (#17365) Review on #17664, and it is a real defect in the helper this PR adds. Both messages were plain template literals, so they were built at the call to `waitForTicks` -- before the first tick. When an emission changes during polling but never matches, the timeout reported the payload present BEFORE the wait rather than the last one actually seen. These helpers exist to make an instrumentation-timing failure legible. A diagnostic that describes the pre-wait state names the one moment that is never the interesting one -- the same shape as the class this PR is about, in the tool built to explain it. `waitForTicks` now takes `string | (() => string)` and resolves it at throw time. Plain strings still work, so no caller is forced to change; the two emit helpers pass thunks. MY FIRST TEST FOR THIS WAS VACUOUS, and the mutation run is the only reason I know. Its stub pushed a new emission on EVERY read, so the eager build also observed a changed value and the test passed against the bug it targeted. The stub now holds its first answer stable and changes only afterwards, which is what makes eager and lazy distinguishable at all. Verified: baseline 10 passed; reverting the `waitForLastEmit` thunk to a plain template literal fails exactly 1 of 10 and names it; restore clean. vue-tsc 0 errors, oxlint clean. Refs #17365
… nothing (#17365) The second review thread on #17664, filed Trivial. It is not trivial — it is the same vacuity as the diagnostic test in the commit before this one, in the fixture that one inherited. `lateEmitter.wrapper.emitted()` incremented `ticks` and pushed the emission during each READ, and `advance` -- the function that would have driven the clock independently -- was never called. Zero occurrences. So reading was what made time pass. A `waitForTicks` implemented as a tight loop calling `emitted()` with no `await nextTick()` would have reached the emission and passed, because the poll IS the clock. The test claimed to prove the helper yields to `nextTick` and could not distinguish yielding from spinning. Reads are pure now, and `advance()` runs concurrently on real ticks. If the helper does not yield, `advance` never progresses and the helper times out. Verified by mutation, which is the only thing that could have shown this: removing `await nextTick()` from `waitForTicks` now fails 1 of 10. Against the old fixture the same mutation passed. That is two vacuous tests in this file in one evening, found by two different routes -- one by my own mutation run, one by a reviewer. Both were correct assertions in a world constructed so the answer could not differ. Reading the assertion finds neither. Refs #17365
…17365) #17365 reports two tests failing only under `--coverage`, and attributes it to v8 instrumentation slowing those two assertions. Measured before fixing, and both halves of that are wrong in ways that change the fix. THE NAMED TESTS PASS UNDER COVERAGE IN ISOLATION. vitest run --coverage <the two named files> 14 passed So instrumentation-on-those-files is not the variable. THE FULL SUITE UNDER COVERAGE FAILED A THIRD TEST, not either named one: Tests 1 failed | 3417 passed | 1 skipped FAIL WorkflowCanvas.contextMenu.test.ts > re-anchors and re-targets when opened again on a different node AssertionError: expected [ 'n1' ] to deeply equal [ 'n2' ] `n1` is the PREVIOUS emission. The menu had re-anchored to `n2`, but `.at(-1)` read the old value because the new one had not happened yet. Nothing about the assertion is wrong; it was made too early. So the MECHANISM in the issue is right -- an assertion racing the component's reaction -- and the SCOPE is not. It is not those two tests. It is whichever timing-sensitive assertion loses the race in a given run, which is why #10323 fixed this once per-test and it came back in different places. WHAT CHANGED. `await trigger(...)` and `firePointer` each flush exactly ONE Vue tick. Every assertion that reads `emitted()` for its LATEST value straight afterwards is racing. There are eight such assertions in `components/workflow/__tests__` (of 64 emission assertions across the frontend suite; the other 56 assert that something was emitted at all, which does not race the same way). All eight now wait for the emission first. `emitTestUtils.ts` waits on TICKS, not on time, and that distinction is the point: a `setTimeout` or a fixed delay trades one race for another -- passing on an idle machine and failing on a loaded one, which is the behaviour being removed. Fifty microtask flushes cost microseconds when idle and are still fifty under load. The bound exists to fail a broken component, not to bound a slow one. The helper is itself tested, 8 cases, including that it THROWS when the state never arrives. Without that, a helper returning immediately would leave every call site passing exactly as today and the flake would return with no failing test to say the guard had stopped guarding. WHAT THIS DOES NOT FIX, and cannot. One of the two originally-named tests is a NEGATIVE assertion -- "does not refetch and keeps showing the pre-edit name". There is no state to wait for, and waiting longer makes a negative assertion MORE likely to fail, not less. That half of the class needs a deterministic flush and then an assertion, which is a different technique; left alone and recorded on the issue rather than papered over. A PASS IS NOT PROOF HERE. The flake is load-dependent and rare, so a green run does not demonstrate it is gone. What is demonstrated: the racing construct is absent from all eight sites, and the helper behaves under test. I would rather state that than claim a fix I cannot evidence. the three affected suites 31 passed emitTestUtils.test.ts 8 passed Refs #17365, #10323
…e mechanism (#17365) The branch carrying this had one commit from 2026-09-25 and no PR. What it contained was good and did not close the issue: it built the `emitTestUtils` helper and applied it to `WorkflowCanvas.contextMenu`, `.marquee` and `.undoRedo` -- three tests the issue does not mention -- while BOTH tests it does name were untouched. WorkflowCanvas.panClick.test.ts > does not swallow the click after a pan OrgChart.contactEdit.test.ts > does not refetch and keeps showing ... That helper's own docstring says why it stopped there: a negative assertion has no state to wait for, and waiting longer makes a false pass MORE likely rather than less. Both named tests read as negatives, so both were left. One of them is not actually a negative, and the other has a positive hiding inside it. PANCLICK IS A POSITIVE WEARING A NEGATIVE TITLE. "does not swallow the click" asserts `expect(selections(w)).toEqual([['ceo']])` -- an emission that must HAVE happened. `waitForEmitCount(w, 'node-selected', 1)` applies directly. The assertion is not weakened: the count is still exactly one, and a component that genuinely swallows the click never reaches it and fails on the bound. ORGCHART IS A REAL NEGATIVE, SO ANCHOR IT. Its two assertions fail for opposite reasons when the flush is short: the error text is a positive that has not rendered yet, and `involvedCallCount()` is a negative that would PASS wrongly if a refetch were still in flight. Waiting cannot fix the second -- but the first is the signal that the failure path RAN TO COMPLETION. Wait for the rendered error, and "no refetch happened" becomes a settled fact about a finished path rather than a race against an unfinished one. That pattern is the structural half the issue asks about (ask 2), so it does not live in one view's test folder. `waitForTicks` moves to `src/test/utils/waitForState.ts` alongside a named `waitForSettled`, and `emitTestUtils` re-exports it so the three tests already using it are unchanged. Fixing each instance where it appeared is what let the class recur through #10323, then #17365 in two more places. Ticks and not `vi.waitFor`: that, `setTimeout` and a fixed delay are all interval-based, so they pass on an idle machine and fail on a loaded one, which is the property being removed. Flushing microtasks is load-independent. VERIFIED UNDER THE CONDITION THAT FAILS, not just under the tests in isolation: npm run test:coverage -> 305 files, 3467 passed, 1 skipped, 0 FAILED The issue reported 2 failed / 3408 passed for this same command. One thing that run also exposed, filed as #17638 rather than fixed here: it exits 1 with zero failing tests, because `MockWebSocket` leaks its open-timer past teardown and vitest counts the resulting `ReferenceError: WebSocket is not defined` as `Errors 1 error`. Same class -- scheduling decides the outcome -- different file and different mechanism, and it is a shared mock many suites use. Not caused by this change, which touches nothing WebSocket-related. Refs #17365
… back under the header (#17365) `oxlint . -D correctness` failed before vitest started, so no test ran: WorkflowCanvas.contextMenu.test.ts:1:10 Identifier 'waitForEmitCount' is imported but never used. Left over from migrating this file to `emitTestUtils`: `waitForEmitCount` occurs exactly once in the file, the import itself. One cause, two red checks -- `frontend-tests` and `Unit & Integration Tests` both run the lint step. Also moved the import BELOW the licence header. It was at line 1, above the copyright, unlike every other file in the tree -- the signature of an automated edit inserting at line 1. 87's observation is why it survived review: nobody reads line 1 when the file appears to start at line 2. Checked the other two files the same migration touched; both have the header first, so this was isolated. Not the #17365 flake class, on three grounds: no test executed, the shape is a static diagnostic at a fixed line, and it fails identically every run. Verified: oxlint clean, and 16 passed on three consecutive runs of this file. The first of those runs failed once on `Cannot call trigger on an empty DOMWrapper` -- the flake class, in a single-file run, which is worth recording because I had previously only observed it in the full suite. Refs #17365
…wait (#17365) Review on #17664, and it is a real defect in the helper this PR adds. Both messages were plain template literals, so they were built at the call to `waitForTicks` -- before the first tick. When an emission changes during polling but never matches, the timeout reported the payload present BEFORE the wait rather than the last one actually seen. These helpers exist to make an instrumentation-timing failure legible. A diagnostic that describes the pre-wait state names the one moment that is never the interesting one -- the same shape as the class this PR is about, in the tool built to explain it. `waitForTicks` now takes `string | (() => string)` and resolves it at throw time. Plain strings still work, so no caller is forced to change; the two emit helpers pass thunks. MY FIRST TEST FOR THIS WAS VACUOUS, and the mutation run is the only reason I know. Its stub pushed a new emission on EVERY read, so the eager build also observed a changed value and the test passed against the bug it targeted. The stub now holds its first answer stable and changes only afterwards, which is what makes eager and lazy distinguishable at all. Verified: baseline 10 passed; reverting the `waitForLastEmit` thunk to a plain template literal fails exactly 1 of 10 and names it; restore clean. vue-tsc 0 errors, oxlint clean. Refs #17365
… nothing (#17365) The second review thread on #17664, filed Trivial. It is not trivial — it is the same vacuity as the diagnostic test in the commit before this one, in the fixture that one inherited. `lateEmitter.wrapper.emitted()` incremented `ticks` and pushed the emission during each READ, and `advance` -- the function that would have driven the clock independently -- was never called. Zero occurrences. So reading was what made time pass. A `waitForTicks` implemented as a tight loop calling `emitted()` with no `await nextTick()` would have reached the emission and passed, because the poll IS the clock. The test claimed to prove the helper yields to `nextTick` and could not distinguish yielding from spinning. Reads are pure now, and `advance()` runs concurrently on real ticks. If the helper does not yield, `advance` never progresses and the helper times out. Verified by mutation, which is the only thing that could have shown this: removing `await nextTick()` from `waitForTicks` now fails 1 of 10. Against the old fixture the same mutation passed. That is two vacuous tests in this file in one evening, found by two different routes -- one by my own mutation run, one by a reviewer. Both were correct assertions in a world constructed so the answer could not differ. Reading the assertion finds neither. Refs #17365
5ff81ec to
7e3667d
Compare
1 participant
Thinking Path
This is a partial improvement on an open class, not a fix for it.
Refs, neverCloses— per the owner's ruling, and #17365 is already retitled to "assertions that pass alone and fail under instrumentation — a class, not two tests".The branch it grew from fixed three tests #17365 does not mention and neither test it names. Both named tests are fixed here. But three full
--coverageruns on this same commit produced failing sets of 5, 2 and 6 files — union 10, intersection empty, and all of them pass in isolation. One of those runs failedWorkflowCanvas.panClick.test.tson a different test threeitblocks below the one fixed here, with the identicalexpected [] to deeply equal [['ceo']].So per-test fixing cannot converge: the list of instances is not the issue's list. That evidence is on the issue.
What Changed
src/test/utils/waitForState.ts(new) —waitForTicksplus a namedwaitForSettled. Ticks, not intervals:vi.waitFor/setTimeoutpass on an idle machine and fail on a loaded one, which is the property being removed.emitTestUtils.tsre-exports it, so the three existing callers are unchanged.WorkflowCanvas.panClick.test.ts— "does not swallow the click" is a positive assertion wearing a negative title: it requires an emission to have happened.waitForEmitCountapplies directly.OrgChart.contactEdit.test.ts— a real negative, so anchored: its two assertions fail for opposite reasons when the flush is short. The rendered error is the signal the failure path completed; once observed, "no refetch" is a settled fact rather than a race.Verification
npm run test:coverageon the rebased branch: 305 files, 3467 passed, 1 skipped, 0 failed, against the issue's reported 2 failed / 3408.Local runs are not evidence about CI and this one is no exception — the suite's own banner says so (
dependency-floor-reporter.ts:120), local node is v24.16.0 against CI's'22', and 51 packages sit below whatpackage.jsondeclares. CI is the authority.Also found and filed while verifying, not fixed here: #17638 —
test:coverageexits 1 with zero failing tests, becauseMockWebSocketleaks its open timer past teardown and vitest counts theReferenceErrorasErrors 1 error. Measured at 3 of 6 runs on pristine main.Refs #17365
Refs #17638
Model Used
Claude Opus 5 (1M context)
Summary by CodeRabbit