fix: re-enable skipped Heart tests and make non-asserting checks assert#7845
Open
voidmatcha wants to merge 1 commit into
Open
fix: re-enable skipped Heart tests and make non-asserting checks assert#7845voidmatcha wants to merge 1 commit into
voidmatcha wants to merge 1 commit 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.
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.