test(qwik-e2e): assert non-asserting e2e checks#8777
Merged
wmertens merged 1 commit intoJun 28, 2026
Conversation
Replace an always-true toBeDefined on a getAttribute result and a bare isVisible call with web-first assertions so the checks can actually fail. Re-targets QwikDev#8727 onto build/v2 at wmertens' request.
|
@qwik.dev/core
@qwik.dev/router
eslint-plugin-qwik
create-qwik
@qwik.dev/optimizer
commit: |
wmertens
enabled auto-merge
June 28, 2026 20:35
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.
What is it?
Description
Two e2e checks in the qwik-e2e suite silently always pass, so they cannot catch a regression:
getAttribute returns string | null and both pass toBeDefined(), so the check never fails.
The sync-qrl app sets the attribute to String(e.defaultPrevented), and the test intent is to
confirm the default was prevented, so this now asserts toHaveAttribute('prevented', 'true').
nothing. Replaced with the web-first await expect(totalIdsLocator).toBeVisible().
Both edits strengthen existing checks without changing test names or behavior. Test-only change,
no published package touched, so no changeset is needed.
This re-targets #8727 onto build/v2 at wmertens' request: that PR was approved on main
but closed because development is focused on build/v2 and merging would conflict. The same
anti-patterns are present in the restructured v2 suite, so the fix is reproduced here.
Testing
Verified statically: the new assertions use Playwright matchers valid for this suite, and the
sync-qrl change matches the app component that sets the attribute to String(e.defaultPrevented).
The e2e specs were not run locally, since the run needs pnpm install plus a core build and a
browser download. This is disclosed rather than claimed.
Found while reviewing the test suite with
e2e-skills/e2e-reviewer.