fix: re-enable skipped Heart tests and make non-asserting checks assert#7845
Open
voidmatcha wants to merge 2 commits into
Open
fix: re-enable skipped Heart tests and make non-asserting checks assert#7845voidmatcha wants to merge 2 commits into
voidmatcha wants to merge 2 commits into
Conversation
- Remove a committed `it.only` (introduced in coder#7539) that silently skipped the 8 sibling Heart unit tests in CI, and repair the "isActive rejects" test that stopped passing while it was skipped (the timer callback's rejection handler runs on the microtask queue; the assertion ran first). - Convert 16 one-shot `page.isVisible()` reads in the e2e suite to web-first `await expect(locator).toBeVisible()/.not.toBeVisible()` — the one-shot form resolves immediately with no auto-retry. - Add the missing matcher to four `expect(await page.isVisible(...))` calls in login.test.ts that asserted nothing, and to a dangling `getByText()` locator in extensions.test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
code-asher
reviewed
Jun 11, 2026
Member
There was a problem hiding this comment.
Thank you for catching these!!
I think the nvm, .not.toBeVisible() ones will fail because locator will time out (since the element is not present) but otherwise it looks good to me.locator is the one that returns immediately, I was thinking of the getBy ones.
I feel like there has got to be a less janky way to wait for the microtask queue haha but I can look into that later.
Edit: hmm actually the ones that are visible are also failing. Not really sure why.
Edit 2: oh just the login test ones are failing.
The required password input blocks empty or cleared submissions
client-side, so the missing-password error and the rate limiter were
unreachable from the UI (the CI failure videos show the browser's native
validation bubble at the moment of the assertion):
- assert the native validation guard (input.password:invalid) for the
missing-password case and rename the test to match the actual behavior
- refill the password before every attempt in the rate limiter test so
all 15 attempts actually POST, and mark it test.slow() since each
attempt now costs a real argon2 hash
- replace the 3x Promise.resolve() microtask drain in heart.test.ts with
a single real-macrotask yield via jest.requireActual("timers")
.setImmediate, which drains the queue deterministically
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
voidmatcha
force-pushed
the
fix/e2e-non-asserting-checks
branch
from
d81c14d to
e9b15b2
Compare
Author
|
The password input is 
|
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
2 participants
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.
This PR fixes test checks that cannot fail:
it.onlycommitted in Add idle timeout #7539 (test/unit/node/heart.test.ts): silently skipped the other 8 Heart tests in CI for the past 7 months. One of them stopped passing while disabled (its rejection handler runs on the microtask queue, after the assertion), re-enabled and repaired with a microtask drain.expect()with no matcher (login.test.ts, 4×): asserted nothing; the error-message tests passed whether or not the message rendered.extensions.test.ts):await page.getByText(...)with no assertion.page.isVisible()reads (downloads/uploads/github/codeServer, 16×): returns immediately with no auto-retry; converted to web-first assertions:No assertions were removed and no behavior added. Every change strengthens an existing check.
Verification:
./ci/dev/test-unit.sh test/unit/node/heart.test.ts- 9/9 pass;tsc --noEmit+ repo ESLint clean on changed files. The Playwright e2e suite was not run locally (requires a full code-server build), the e2e changes are mechanical assertion-form conversions. Happy to iterate if CI surfaces anything.Found while reviewing the test suite with
e2e-skills/e2e-reviewer, which flags committed.onlyleaks, one-shot visibility reads, and matcher-lessexpect()calls.