Add happy-dom test project alongside the browser matrix#333
Merged
brianmhunt merged 8 commits intomainfrom Apr 20, 2026
Merged
Conversation
Introduces a `cli-happy-dom` vitest project that runs the full spec suite under happy-dom, expanding coverage to non-browser runtimes (SSR / TUI / headless contexts). The authoritative real-browser matrix (chromium / firefox / webkit) is untouched and still runs every spec. Real fixes surfaced by the exercise: - Modernize `triggerEvent` to prefer `new MouseEvent/KeyboardEvent/Event(...)` over the deprecated `document.createEvent` / `initEvent` path. Synthetic clicks now toggle checkbox `.checked` in happy-dom as they do natively. - Drop legacy `event.cancelBubble = true` from the event handler — readonly in happy-dom and redundant with the `stopPropagation()` that follows. - Harden `dummyTemplateEngine` eval against bundler-renamed closure vars by passing dependencies explicitly via `new Function(...)`. - Use `Object.prototype.toString.call(node)` instead of `'' + node` in `ComponentProvider` for unknown-element detection. - Replace legacy `window.testDivTemplate` named-access with the existing `ensureNodeExistsAndIsEmpty` helper in the template spec. Where divergences are genuine env gaps (e.g. `<select>` auto-selection, `focus()`/`activeElement` semantics, `setTimeout` errors routing through `window.onerror`), tests use `it.skipIf(isHappyDom())` with a brief note on the gap, per the additive-coverage framing in AGENTS.md. Also adjusts `innerText` assertions in a few specs to tolerate whitespace rendering differences between DOM implementations. Result: cli-happy-dom green (0 failed, 2676 passed, 64 skipped); browser matrix unchanged (2698 passed, 42 skipped, 0 regressions). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Restore `relatedTarget: element` on synthetic MouseEvents; the previous modernization dropped it but the legacy `initEvent(...)` call had passed it as the last arg, and consumers read `event.relatedTarget` on mouseover / mouseout / mouseenter / mouseleave. - Narrow the foreach `focus` skip from `describe.skipIf` to per-test `it.skipIf` on the four tests with genuine happy-dom gaps. The first test (`does not preserve the target on apply bindings`) passes in happy-dom — the describe-level skip was silently removing verified coverage. - Add a changeset covering the `triggerEvent`, `eventHandler`, and `ComponentProvider` behavior deltas from the previous commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModernizes synthetic event construction to use modern event constructors, changes bubbling logic to always call stopPropagation, improves HTMLUnknownElement detection, adds test-environment detectors and browser-only test wrappers, converts many DOM-sensitive tests to browser-only, and updates Vitest config and CI to run browser and happy-dom projects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3aea21c88a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+35
to
+36
| include: ALL_SPECS, | ||
| environment: 'node', |
There was a problem hiding this comment.
This project is configured to run ALL_SPECS under environment: 'node', but the suite includes many DOM/browser specs (including builds/knockout files that now call isHappyDom() from global setup). Because CI workflows run plain bunx vitest run (checked in .github/workflows/main-build.yml and test-headless.yml), Vitest will execute this new project too; Vitest’s projects guide also notes that if you want a single project you must pass --project, so the default run now picks up cli-bun and fails before browser coverage can complete. Restrict this project’s include to node-safe specs (or keep it opt-in) before enabling it in the shared config.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/binding.template/helpers/dummyTemplateEngine.ts (1)
70-83:⚠️ Potential issue | 🟡 MinorSubtle completion-value semantics change vs.
eval.
eval(script)returns the completion value of the last statement, so a multi-statement script whose last clause is a bare expression (e.g.,var x = 5; x) yields that expression. The new fallback path usesnew Function(..., script)with no implicitreturn, so any script that fails the expression wrap returnsundefined→ coerced to''.Existing template scripts in the suite either are pure expressions (handled by the
return (${script})branch) or terminate with; undefined;(matches''intentionally), so this is consistent with the reported 0-failure result. Just flagging for visibility in case future[js:…]scripts rely on the old last-expression-value behavior — if that becomes necessary, the fallback could try'return (' + lastExpr + ')'or simply require an explicitreturn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.template/helpers/dummyTemplateEngine.ts` around lines 70 - 83, The fallback path in evalHandler currently constructs a new Function directly with the raw script, which loses eval-like completion-value semantics for multi-statement scripts; update the second new Function (the fallback in evalHandler) to preserve the completion value by wrapping the original script in an IIFE and returning its result (i.e., create the Function so it returns the IIFE's value instead of executing the script bare), ensuring the same arguments ('unwrap', 'nomangle$data', 'rt_options', 'bindingContext') are used and preserving the existing try/catch and result-to-string conversion.
🧹 Nitpick comments (7)
packages/utils/src/dom/event.ts (1)
89-101: Event-category mapping looks correct; couple of small follow-ups.
focusin/focusout(used byhasfocus.ts:67-73) are intentionally NOT inknownEventTypesByEventName, so they fall through to the genericEventconstructor on line 100. That's the right call for the bubbling pair (focus/blurwould needFocusEvent-style non-bubbling behavior anyway), but worth a one-line comment inknownEventsso a future contributor doesn't "fix" this by adding them under'UIEvents'and accidentally swap them onto theKeyboardEventbranch.view: options.global— if a consumer ever setsoptions.globalto something that isn't aWindow(e.g.,globalThisin a Node-ish runtime), some DOM impls throw on theMouseEventInit.viewsetter. Defaulting toview: undefinedwhenoptions.globalisn't aWindowwould be safer:♻️ Defensive view init
- const view = options.global as Window | undefined + const g = options.global as unknown + const view = (typeof Window !== 'undefined' && g instanceof Window ? g : undefined) as Window | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/dom/event.ts` around lines 89 - 101, Add a one-line clarifying comment in the knownEventTypesByEventName map noting that focusin/focusout are intentionally omitted (they should fall through to the generic Event and not be mapped to UIEvents/FocusEvent), and change the view initialization in the event creation block to only use options.global when it is actually a Window (i.e., set view = options.global if instanceof Window, otherwise undefined) so MouseEventInit.view won't throw in non-Window runtimes; update the references in the branch that constructs new MouseEvent(eventType, { bubbles: true, cancelable: true, view, relatedTarget: element }) and the code path using KeyboardEvent/Event accordingly.vitest.config.ts (1)
11-13: Remove root-level test config duplicated in projects.The root-level
testTimeout: 10000andglobals: true(lines 11–13) are not inherited by the projects because none of them useextendsconfiguration. Since thebrowserproject explicitly sets both (line 20) andcli-bun/cli-happy-domredundantly setglobals: truewithout inheriting from root, these root-level settings are dead code.♻️ Cleanup
export default defineConfig({ test: { - testTimeout: 10000, - globals: true, projects: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 11 - 13, The root-level test config contains redundant keys test.testTimeout and test.globals that are not inherited by projects, so remove those two properties from the root "test" object; ensure each project config (e.g., the "browser" project and the "cli-bun"/"cli-happy-dom" projects) explicitly defines the needed settings (testTimeout and/or globals) where required (the "browser" project already sets both), and delete the duplicated root-level testTimeout and globals entries to clean up dead config.packages/utils/helpers/test-env.ts (1)
11-19: SimplifyisRealBrowserternary and remove dead Playwright marker check.Two concerns:
- The
A && B && C ? true : Dternary structure is confusing. It collapses cleanly to boolean logic that reads more clearly.window.PlaywrightTestingLibraryis never injected by Playwright or@vitest/browser-playwright. The first branch of the ternary is dead code —isRealBrowsereffectively only checks the UA-string fallback.♻️ Proposed simplification
export function isRealBrowser(): boolean { - return typeof window !== 'undefined' && - !isHappyDom() && - typeof (window as any).PlaywrightTestingLibrary !== 'undefined' - ? true - : typeof navigator !== 'undefined' && - /Chrome|Firefox|Safari|WebKit/i.test(navigator.userAgent ?? '') && - !isHappyDom() + if (isHappyDom()) return false + if (typeof window === 'undefined' || typeof navigator === 'undefined') return false + return /Chrome|Firefox|Safari|WebKit/i.test(navigator.userAgent ?? '') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/helpers/test-env.ts` around lines 11 - 19, The isRealBrowser() function uses a confusing ternary and includes a dead check for window.PlaywrightTestingLibrary; simplify it by removing the Playwright marker check and replacing the ternary with a single boolean expression that returns whether a real browser is present: check typeof window !== 'undefined' and !isHappyDom(), or fall back to typeof navigator !== 'undefined' && /Chrome|Firefox|Safari|WebKit/i.test(navigator.userAgent ?? '') && !isHappyDom(); update the function body (isRealBrowser) to return that simplified boolean expression and remove the unreachable branch referencing PlaywrightTestingLibrary.packages/binding.component/spec/componentBindingBehaviors.ts (1)
1274-1274: Whitespace normalization is reasonable, but consider consolidating.The three updated assertions now use
.replace(/\s+/g, ' ').trim()while nearby similar checks still use plain.trim()(e.g., lines 1252, 1295, 1316, 1340, 1361). The inconsistency suggests the rendering difference only surfaces in the multi-node/default-slot cases, which is fine, but a tiny helper (e.g.,normalizeInnerText(el)) would make intent explicit and centralize any future happy-dom divergence in one place.Also note line 1462 uses
.replace(/\s+/, ' ')(non-global) — unchanged here, but worth a follow-up so all such normalizations behave consistently.Also applies to: 1408-1408, 1429-1429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.component/spec/componentBindingBehaviors.ts` at line 1274, Create a small helper function (e.g., normalizeInnerText(el) or normalizeText(str)) in the spec file and replace the ad-hoc uses of .replace(/\s+/g, ' ').trim() and the inconsistent .replace(/\s+/, ' ')/.trim() calls with that helper; update the assertions that currently use (testNode.children[0] as HTMLInputElement).innerText.replace(/\s+/g, ' ').trim() (and the other affected sites around the file such as the occurrences at the diffed lines and the ones at 1408, 1429, 1462) to call normalizeInnerText(element) so whitespace normalization is centralized and consistent across multi-node/default-slot cases and single-node checks.builds/knockout/spec/onErrorBehaviors.js (1)
113-115: LGTM. Skipping the two async onError cases under happy-dom is the right call givensetTimeouterrors don't reachwindow.onerrorthere; comments are clear.Optional follow-up: once happy-dom's setTimeout/window.onerror behavior is fixed upstream, consider tracking these skips in an issue so they can be re-enabled.
Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/onErrorBehaviors.js` around lines 113 - 115, Add an issue to track re-enabling the skipped async onError tests and annotate the two skipped tests that use it.skipIf(isHappyDom()) (the tests titled "fires on async component errors" and the similar test at lines ~140-141) with a one-line TODO referencing that issue number; update the test files to include a short comment above each it.skipIf call mentioning the created issue ID so maintainers can re-enable them once happy-dom fixes setTimeout/window.onerror behavior.builds/knockout/helpers/vitest-setup.js (1)
3-14: LGTM — detectors correctly surfaced for Knockout build specs.
isHappyDom/isRealBrowser/isNodeonglobalThisis the minimal bridge these legacy-style specs need to useit.skipIf(...)without importing ESM helpers directly. The comment clarifies intent (document divergence rather than silently exclude).Minor: consider typing these additions on
globalThisvia a small.d.tsambient declaration so TypeScript specs get type checking onisHappyDom()when referenced without import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/helpers/vitest-setup.js` around lines 3 - 14, Add an ambient declaration file that augments the globalThis type to include the three test environment detectors so TypeScript recognizes isHappyDom, isRealBrowser and isNode used in vitest-setup; create a .d.ts (e.g., global-test-env.d.ts) that declares globalThis.isHappyDom: () => boolean, globalThis.isRealBrowser: () => boolean and globalThis.isNode: () => boolean (or appropriate signatures) and ensure the file is included in tsconfig types/include so specs using isHappyDom() without an import get type checking.packages/binding.foreach/spec/eachBehavior.ts (1)
1032-1086: Consider collapsing the four adjacent happy-dom skips into a nesteddescribe.skipIf.All four contiguous tests skip for the same focus/activeElement reason. A single
describe.skipIf(isHappyDom())('re-ordering focus preservation', ...)would avoid repeating theskipIfon everyitand keep the grouping visible. Optional — the current form is already explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.foreach/spec/eachBehavior.ts` around lines 1032 - 1086, Collapse the four adjacent it.skipIf(isHappyDom()) tests into a single describe.skipIf(isHappyDom()) block named e.g. 're-ordering focus preservation' and move the four it(...) tests (the ones starting "does not preserves primitive targets when re-ordering", "preserves objects when re-ordering", "preserves objects when re-ordering multiple identical", and "preserves objects when re-ordering multiple identical, alt") inside that describe, then remove the per-test skipIf wrappers so each test becomes a plain it(...). This keeps isHappyDom() gating while avoiding repeated it.skipIf calls and preserves each test body and assertions (list/remove/push, applyBindings, focus checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vitest.config.ts`:
- Around line 32-39: The cli-bun project configuration currently runs ALL_SPECS
in environment: 'node' but lacks the shared setup
(builds/knockout/helpers/vitest-setup.js) so tests that rely on globalThis.ko,
expect, document, etc., will crash; fix by either setting the cli-bun project's
include to [] (or removing the project), adding an exclude/.skip filter to
prevent accidental runs, or wiring the setupFiles to the shared vitest-setup.js
so globals are initialized; also add a one-line comment in the cli-bun project
block noting it's intentionally disabled/tracked in an issue for clarity.
- Around line 40-50: The vitest config defines a test project named
'cli-happy-dom' with environment: 'happy-dom' but the root package.json is
missing that devDependency; add "happy-dom" to the root devDependencies (e.g.,
run npm/yarn/pnpm add --save-dev happy-dom) so Vitest can resolve the
environment, then commit the updated package.json and lockfile; verify the
project name 'cli-happy-dom' and environment: 'happy-dom' in vitest.config.ts to
ensure they match the installed package.
---
Outside diff comments:
In `@packages/binding.template/helpers/dummyTemplateEngine.ts`:
- Around line 70-83: The fallback path in evalHandler currently constructs a new
Function directly with the raw script, which loses eval-like completion-value
semantics for multi-statement scripts; update the second new Function (the
fallback in evalHandler) to preserve the completion value by wrapping the
original script in an IIFE and returning its result (i.e., create the Function
so it returns the IIFE's value instead of executing the script bare), ensuring
the same arguments ('unwrap', 'nomangle$data', 'rt_options', 'bindingContext')
are used and preserving the existing try/catch and result-to-string conversion.
---
Nitpick comments:
In `@builds/knockout/helpers/vitest-setup.js`:
- Around line 3-14: Add an ambient declaration file that augments the globalThis
type to include the three test environment detectors so TypeScript recognizes
isHappyDom, isRealBrowser and isNode used in vitest-setup; create a .d.ts (e.g.,
global-test-env.d.ts) that declares globalThis.isHappyDom: () => boolean,
globalThis.isRealBrowser: () => boolean and globalThis.isNode: () => boolean (or
appropriate signatures) and ensure the file is included in tsconfig
types/include so specs using isHappyDom() without an import get type checking.
In `@builds/knockout/spec/onErrorBehaviors.js`:
- Around line 113-115: Add an issue to track re-enabling the skipped async
onError tests and annotate the two skipped tests that use
it.skipIf(isHappyDom()) (the tests titled "fires on async component errors" and
the similar test at lines ~140-141) with a one-line TODO referencing that issue
number; update the test files to include a short comment above each it.skipIf
call mentioning the created issue ID so maintainers can re-enable them once
happy-dom fixes setTimeout/window.onerror behavior.
In `@packages/binding.component/spec/componentBindingBehaviors.ts`:
- Line 1274: Create a small helper function (e.g., normalizeInnerText(el) or
normalizeText(str)) in the spec file and replace the ad-hoc uses of
.replace(/\s+/g, ' ').trim() and the inconsistent .replace(/\s+/, ' ')/.trim()
calls with that helper; update the assertions that currently use
(testNode.children[0] as HTMLInputElement).innerText.replace(/\s+/g, ' ').trim()
(and the other affected sites around the file such as the occurrences at the
diffed lines and the ones at 1408, 1429, 1462) to call
normalizeInnerText(element) so whitespace normalization is centralized and
consistent across multi-node/default-slot cases and single-node checks.
In `@packages/binding.foreach/spec/eachBehavior.ts`:
- Around line 1032-1086: Collapse the four adjacent it.skipIf(isHappyDom())
tests into a single describe.skipIf(isHappyDom()) block named e.g. 're-ordering
focus preservation' and move the four it(...) tests (the ones starting "does not
preserves primitive targets when re-ordering", "preserves objects when
re-ordering", "preserves objects when re-ordering multiple identical", and
"preserves objects when re-ordering multiple identical, alt") inside that
describe, then remove the per-test skipIf wrappers so each test becomes a plain
it(...). This keeps isHappyDom() gating while avoiding repeated it.skipIf calls
and preserves each test body and assertions (list/remove/push, applyBindings,
focus checks).
In `@packages/utils/helpers/test-env.ts`:
- Around line 11-19: The isRealBrowser() function uses a confusing ternary and
includes a dead check for window.PlaywrightTestingLibrary; simplify it by
removing the Playwright marker check and replacing the ternary with a single
boolean expression that returns whether a real browser is present: check typeof
window !== 'undefined' and !isHappyDom(), or fall back to typeof navigator !==
'undefined' && /Chrome|Firefox|Safari|WebKit/i.test(navigator.userAgent ?? '')
&& !isHappyDom(); update the function body (isRealBrowser) to return that
simplified boolean expression and remove the unreachable branch referencing
PlaywrightTestingLibrary.
In `@packages/utils/src/dom/event.ts`:
- Around line 89-101: Add a one-line clarifying comment in the
knownEventTypesByEventName map noting that focusin/focusout are intentionally
omitted (they should fall through to the generic Event and not be mapped to
UIEvents/FocusEvent), and change the view initialization in the event creation
block to only use options.global when it is actually a Window (i.e., set view =
options.global if instanceof Window, otherwise undefined) so MouseEventInit.view
won't throw in non-Window runtimes; update the references in the branch that
constructs new MouseEvent(eventType, { bubbles: true, cancelable: true, view,
relatedTarget: element }) and the code path using KeyboardEvent/Event
accordingly.
In `@vitest.config.ts`:
- Around line 11-13: The root-level test config contains redundant keys
test.testTimeout and test.globals that are not inherited by projects, so remove
those two properties from the root "test" object; ensure each project config
(e.g., the "browser" project and the "cli-bun"/"cli-happy-dom" projects)
explicitly defines the needed settings (testTimeout and/or globals) where
required (the "browser" project already sets both), and delete the duplicated
root-level testTimeout and globals entries to clean up dead config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f8a0c98-bcb4-4fc4-9439-6a925ad3820a
📒 Files selected for processing (23)
.changeset/modernize-trigger-event.mdbuilds/knockout/helpers/vitest-setup.jsbuilds/knockout/spec/components/defaultLoaderBehaviors.jsbuilds/knockout/spec/defaultBindings/attrBehaviors.jsbuilds/knockout/spec/defaultBindings/optionsBehaviors.jsbuilds/knockout/spec/defaultBindings/valueBehaviors.jsbuilds/knockout/spec/onErrorBehaviors.jspackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.core/spec/attrBehaviors.tspackages/binding.core/spec/eventBehaviors.tspackages/binding.core/spec/optionsBehaviors.tspackages/binding.core/spec/valueBehaviors.tspackages/binding.core/src/event.tspackages/binding.foreach/spec/eachBehavior.tspackages/binding.if/spec/elseBehaviors.tspackages/binding.template/helpers/dummyTemplateEngine.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/provider.component/src/ComponentProvider.tspackages/utils.component/spec/defaultLoaderBehaviors.tspackages/utils/helpers/test-env.tspackages/utils/spec/utilsDomBehaviors.tspackages/utils/src/dom/event.tsvitest.config.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an additional Vitest runtime target (happy-dom) to run the existing spec suite outside real browsers, plus a handful of DOM/test harness adjustments needed to make the suite meaningful under that environment.
Changes:
- Restructures
vitest.config.tsinto multiple projects (browser matrix + CLI environments) and adds happy-dom-focused environment detection helpers. - Modernizes synthetic event creation (
triggerEvent) and updates event bubbling handling in theeventbinding for better cross-DOM compatibility. - Documents happy-dom divergences by conditionally skipping specific specs, and hardens a few tests/helpers surfaced by the new environment.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Switches to multi-project Vitest config (browser + CLI projects). |
| packages/utils/src/dom/event.ts | Replaces deprecated createEvent/initEvent with modern Event/MouseEvent/KeyboardEvent constructors. |
| packages/utils/spec/utilsDomBehaviors.ts | Adds a happy-dom-specific skip for a selectExtensions behavior. |
| packages/utils/helpers/test-env.ts | Introduces environment detectors (happy-dom / real browser / node). |
| packages/utils.component/spec/defaultLoaderBehaviors.ts | Skips <textarea> template parsing tests under happy-dom. |
| packages/provider.component/src/ComponentProvider.ts | Uses Object.prototype.toString.call(node) for HTMLUnknownElement detection. |
| packages/binding.template/spec/nativeTemplateEngineBehaviors.ts | Removes reliance on window.<id> named access by resolving nodes explicitly. |
| packages/binding.template/helpers/dummyTemplateEngine.ts | Replaces eval closure dependency with parameterized new Function(...) evaluation. |
| packages/binding.if/spec/elseBehaviors.ts | Normalizes innerText assertions to be whitespace-tolerant. |
| packages/binding.foreach/spec/eachBehavior.ts | Skips focus-preservation tests under happy-dom due to divergent focus semantics. |
| packages/binding.core/src/event.ts | Drops cancelBubble mutation; relies on stopPropagation() for non-bubbling behavior. |
| packages/binding.core/spec/valueBehaviors.ts | Skips a multi-select behavior test under happy-dom due to selectedIndex differences. |
| packages/binding.core/spec/optionsBehaviors.ts | Skips several <select>-semantics tests under happy-dom. |
| packages/binding.core/spec/eventBehaviors.ts | Imports environment helper (but currently unused). |
| packages/binding.core/spec/attrBehaviors.ts | Skips namespaced attribute test under happy-dom (namespace resolution gap). |
| packages/binding.component/spec/componentBindingBehaviors.ts | Makes text assertions whitespace-normalizing for env variance. |
| builds/knockout/spec/onErrorBehaviors.js | Skips async onerror tests under happy-dom due to timer error routing differences. |
| builds/knockout/spec/defaultBindings/valueBehaviors.js | Skips a multi-select behavior test under happy-dom in the legacy build suite. |
| builds/knockout/spec/defaultBindings/optionsBehaviors.js | Skips select/options behavior tests under happy-dom in the legacy build suite. |
| builds/knockout/spec/defaultBindings/attrBehaviors.js | Skips namespaced attribute test under happy-dom in the legacy build suite. |
| builds/knockout/spec/components/defaultLoaderBehaviors.js | Skips <textarea> template tests under happy-dom in the legacy build suite. |
| builds/knockout/helpers/vitest-setup.js | Exposes environment detectors on globalThis for legacy JS specs. |
| .changeset/modernize-trigger-event.md | Changeset documenting the behavioral/compat adjustments across packages. |
Comment on lines
+26
to
+39
| // EXPERIMENTAL: additive CLI coverage — Bun runtime, no DOM. | ||
| // We run the suite via `bunx vitest`, so the runtime is already Bun. | ||
| // `environment: 'node'` just tells vitest "don't provide a DOM" — | ||
| // Bun provides the node-compatible globals natively. Intent: prove | ||
| // the reactive primitives run in server-side contexts (Bun CLIs, | ||
| // TUIs, daemons). | ||
| { | ||
| test: { | ||
| name: 'cli-bun', | ||
| include: ALL_SPECS, | ||
| environment: 'node', | ||
| globals: true | ||
| } | ||
| }, |
There was a problem hiding this comment.
bunx vitest run (used by bun run test and both test-headless.yml + main-build.yml) will run all configured projects by default. With cli-bun included here (and PR description noting it fails broadly), CI and local bun run test will fail; additionally the browser matrix workflow would rerun the cli-happy-dom suite three times (once per browser). Consider removing cli-bun from the default config, or gating it behind an env flag, and/or updating CI/scripts to explicitly run --project browser for the matrix plus a separate job for cli-happy-dom.
| import { bindings as coreBindings } from '../dist' | ||
|
|
||
| import { prepareTestNode } from '../../utils/helpers/mocha-test-helpers' | ||
| import { isHappyDom } from '../../utils/helpers/test-env' |
There was a problem hiding this comment.
isHappyDom is imported here but never used in this file, which will fail Biome CI due to an unused import. Remove the import, or add the intended it.skipIf(isHappyDom()) usage in this spec.
Suggested change
| import { isHappyDom } from '../../utils/helpers/test-env' |
Comment on lines
+173
to
+175
| // happy-dom gap: `selected` attribute on an <option> parsed via innerHTML | ||
| // does not set selectedIndex the way real browsers do. | ||
| it.skipIf(isHappyDom())('should use loose equality for select value', () => { |
There was a problem hiding this comment.
Using it.skipIf(...) in TypeScript specs will fail bunx tsc with the current repo typings (tsconfig.json restricts global test types to @types/mocha, which doesn't define skipIf). Either add an ambient type augmentation for Mocha.TestFunction to include skipIf, or rewrite these conditionals using if (isHappyDom()) it.skip(...) else it(...) to stay Mocha-typed.
- Remove the `cli-bun` vitest project. Running `bunx vitest run` without
`--project` runs every configured project, which means the default
`bun run test` and both CI workflows (main-build.yml, test-headless.yml)
would execute the broken cli-bun config. Restoring it properly requires
a per-package runtime declaration (e.g. `"tko": { "runtime": "dom" |
"universal" }`) so the runner can include only DOM-free packages — that's
a separate follow-up branch.
- Remove the unused `isHappyDom` import from eventBehaviors.ts. The source
fix to `event.cancelBubble` removed the need to skip the bubble tests;
the import was left behind.
- Rewrite `it.skipIf(isHappyDom())` / `describe.skipIf(isHappyDom())` in
TypeScript spec files as `;(isHappyDom() ? it.skip : it)(...)`. The repo
types test globals via `@types/mocha`, which doesn't declare `skipIf` on
`TestFunction`, so `bunx tsc` rejected the shorter form. The ternary uses
only Mocha-native `it` and `it.skip` and makes the selection explicit at
the call site. JS build specs keep `it.skipIf(...)` — no typechecker to
satisfy there.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the three CI failures on PR #333: - Add `happy-dom` to devDependencies. It's an optional peerDependency of vitest, so local installs had it transitively but `bun install --frozen-lockfile` in CI did not — every `cli-happy-dom` worker crashed with `Cannot find package 'happy-dom'`. 3× in the browser matrix. - Scope CI vitest invocations: browser-matrix jobs now run `--project browser` and a new separate `cli-happy-dom` job runs the happy-dom project once. Previously `bunx vitest run` with no project flag ran every configured project in every matrix job. - Reflow test files whose `(isHappyDom() ? it.skip : it)('long title', ...)` call sites exceeded the formatter's line-length limit. `biome ci` is stricter than `biome check` and these format deltas failed the lint-and-typecheck workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/binding.core/spec/optionsBehaviors.ts (1)
187-237: Sameit.skipIfrefactor opportunity applies here.Three tests in this file use the same
;(isHappyDom() ? it.skip : it)(...)pattern. If you adoptit.skipIf(isHappyDom())(...)invalueBehaviors.ts, apply it consistently across the happy-dom gates in this file (lines 189, 208, 310) for readability and to avoid the leading-semicolon requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.core/spec/optionsBehaviors.ts` around lines 187 - 237, Replace the repeated pattern `;(isHappyDom() ? it.skip : it)(...)` with the clearer helper call `it.skipIf(isHappyDom())(...)` for the three tests that gate on happy-dom (the tests whose descriptions start "Should select caption by default and retain selection when adding multiple items" and "Should trigger a change event when the options selection is populated or changed by modifying the options data (single select)" and the third similar gate later in the file), updating each invocation (remove the leading semicolon) so the call becomes `it.skipIf(isHappyDom())('test description', function () { ... })`; keep the test bodies unchanged and ensure you import/retain the `it.skipIf` helper if required by the test harness.packages/binding.core/spec/valueBehaviors.ts (1)
441-466: Consider using Vitest'sit.skipIffor cleaner conditional skipping.The
;(isHappyDom() ? it.skip : it)(...)pattern works but Vitest providesit.skipIf(condition)which is more idiomatic and avoids the leading-semicolon workaround required by the ternary form.♻️ Proposed refactor
- // happy-dom gap: size>1 <select> does not honor selectedIndex = -1 the same way as real browsers. - ;(isHappyDom() ? it.skip : it)( - 'When size > 1, should unselect all options when value is undefined, null, or ""', - function () { + // happy-dom gap: size>1 <select> does not honor selectedIndex = -1 the same way as real browsers. + it.skipIf(isHappyDom())('When size > 1, should unselect all options when value is undefined, null, or ""', function () { const myObservable = observable('B') ... - } - ) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/binding.core/spec/valueBehaviors.ts` around lines 441 - 466, Replace the ternary/leading-semicolon skip pattern "(isHappyDom() ? it.skip : it)(...)" with Vitest's idiomatic conditional skip API by calling it.skipIf(isHappyDom()) with the same test description and callback; update the invocation around the test whose description starts "When size > 1, should unselect all options when value is undefined, null, or \"\"" so it uses it.skipIf(isHappyDom()) and remove the leading-semicolon workaround.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/binding.core/spec/optionsBehaviors.ts`:
- Around line 187-237: Replace the repeated pattern `;(isHappyDom() ? it.skip :
it)(...)` with the clearer helper call `it.skipIf(isHappyDom())(...)` for the
three tests that gate on happy-dom (the tests whose descriptions start "Should
select caption by default and retain selection when adding multiple items" and
"Should trigger a change event when the options selection is populated or
changed by modifying the options data (single select)" and the third similar
gate later in the file), updating each invocation (remove the leading semicolon)
so the call becomes `it.skipIf(isHappyDom())('test description', function () {
... })`; keep the test bodies unchanged and ensure you import/retain the
`it.skipIf` helper if required by the test harness.
In `@packages/binding.core/spec/valueBehaviors.ts`:
- Around line 441-466: Replace the ternary/leading-semicolon skip pattern
"(isHappyDom() ? it.skip : it)(...)" with Vitest's idiomatic conditional skip
API by calling it.skipIf(isHappyDom()) with the same test description and
callback; update the invocation around the test whose description starts "When
size > 1, should unselect all options when value is undefined, null, or \"\"" so
it uses it.skipIf(isHappyDom()) and remove the leading-semicolon workaround.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ec34b44-04dd-4611-87a1-380cc9aab018
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/main-build.yml.github/workflows/test-headless.ymlbuilds/knockout/spec/defaultBindings/optionsBehaviors.jsbuilds/knockout/spec/defaultBindings/valueBehaviors.jspackage.jsonpackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.core/spec/optionsBehaviors.tspackages/binding.core/spec/valueBehaviors.ts
✅ Files skipped from review due to trivial changes (2)
- package.json
- packages/binding.component/spec/componentBindingBehaviors.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- builds/knockout/spec/defaultBindings/optionsBehaviors.js
The previous `;(isHappyDom() ? it.skip : it)(...)` form forced biome's
formatter to reflow long test calls across multiple lines, breaking the
indent visually compared to sibling `it(...)` tests. This made the diff
noisy to review.
Introduce `itBrowserOnly` and `describeBrowserOnly` in test-env.ts:
export const itBrowserOnly = (isHappyDom() ? it.skip : it) as typeof it
Call sites read as semantic labels — `itBrowserOnly('title', fn)` — and
keep the same one-line shape as `it('title', fn)`. The conditional is
evaluated once at module init, not per test call; fine since env can't
change mid-run.
A handful of tests with titles long enough to push past biome's 120-char
line-width get a one-line `// biome-ignore format: keep test-call on a
single line for indent consistency` directive. Biome's test-call
carve-out recognizes literal `it`/`test`/`describe` names but not
user-defined wrappers, so the pragma is the cleanest path to consistent
indent for those specific long-titled tests.
Also exposed the label globals via builds/knockout/helpers/vitest-setup.js
so the .js build specs can use the same name (no imports needed there).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous diff traded test strength for env portability: three
assertions in componentBindingBehaviors.ts and three in elseBehaviors.ts
were weakened from `.innerText.trim()` to `.innerText.replace(/\s+/g, '
').trim()` (or added a new `.trim()` where exact-equal had been used) to
accommodate happy-dom's divergent innerText whitespace rendering. That
hid regression signal from real browsers — any template emitting spurious
whitespace would now pass silently.
Restore the original assertions and skip the affected tests in happy-dom
with an inline guard:
it('name', function (ctx: any) {
if (!isRealBrowser()) return ctx.skip('happy-dom: innerText whitespace rendering differs from real browsers')
...
})
Notes on mechanics:
- Vitest doesn't bind Mocha's `this` to test callbacks; the skip API is
on the context argument (first parameter). `ctx: any` typed because
@types/mocha declares that slot as `Done` for async callbacks.
- One assertion (`processes default and named slots`, line 1386) genuinely
needed the whitespace normalization in real browsers too (pre-existing
on main) — restored that form and added a brief comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
We had two patterns doing the same thing — `itBrowserOnly(...)` wrapper
labels (18 sites) and inline `if (!isRealBrowser()) return ctx.skip(...)`
guards (6 sites, added for the innerText reversions). Two patterns for
one concept is noise; pick one and use it everywhere.
Consolidate on the inline form. Every browser-only test now reads:
it('name', function (ctx: any) {
if (!isRealBrowser()) return ctx.skip('happy-dom: reason')
// body
})
What this removes:
- `itBrowserOnly` / `describeBrowserOnly` exports from test-env.ts
- their global exposure in builds/knockout/helpers/vitest-setup.js
- three `// biome-ignore format: keep test-call on a single line` pragmas
(biome's test-call carve-out handles plain `it(...)` without help)
- the freestanding `// happy-dom gap:` comments above each test (the
reason now lives on the `ctx.skip(...)` call, which also surfaces it
in vitest's verbose reporter)
Also cleans up `isRealBrowser` in test-env.ts: the previous definition
used a tangled ternary with a `window.PlaywrightTestingLibrary` check
and a browser-UA regex that JSDOM could match. Simplified to
`!isHappyDom() && !isNode()`, which is what we actually mean and is
consistent with how `isRealBrowser()` is used in the specs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`if (!isRealBrowser()) return ctx.skip('happy-dom: reason')` was a
double-negative detour — the check says "not a real browser" but the
skip reason always starts with "happy-dom:". Say what we mean directly.
All 24 guards now read:
it('name', function (ctx: any) {
if (isHappyDom()) return ctx.skip('happy-dom: reason')
// body
})
Also drops `isRealBrowser` and `isNode` from test-env.ts (neither had a
real consumer — `isNode` was scaffolding for the deferred `cli-bun`
project, and `isRealBrowser` was the double-negated wrapper of
`isHappyDom`). When we bring back a node-env project we can add them
back alongside the package.json runtime declaration scheme.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
cli-happy-domvitest project that runs the full spec suite under happy-dom, expanding coverage to non-browser runtimes (SSR / TUI / headless). The authoritative real-browser matrix (chromium / firefox / webkit) is unchanged.triggerEvent(now usesnew MouseEvent/KeyboardEvent/Event(...)instead of deprecatedcreateEvent/initEvent) and theeventbinding's bubble handling (drops readonly-in-happy-domcancelBubble = truethat was redundant withstopPropagation()).it.skipIf(isHappyDom())and a short note on the gap, per the additive-coverage framing in AGENTS.md.Results: cli-happy-dom green (0 failed / 2677 passed / 63 skipped). Browser matrix unchanged (2698 passed / 42 skipped / 0 regressions). Changeset included for the
@tko/utils/@tko/binding.core/@tko/provider.componentbehavior deltas.Context
Per AGENTS.md, new test environments are additive — they expand coverage for runtimes TKO should work in, never replace the browser matrix. Failures in a new environment are signal, handled one of three ways:
window.<id>named access)skipIf(where the env genuinely lacks a DOM feature)Real fixes (1) + test fixes (2) + documented gaps (3) are distinguished in the diff by code location and the leading comment on the skip.
Real fixes
packages/utils/src/dom/event.ts—triggerEventmodernized. Still passesrelatedTarget: elementfor mouse events to preserve legacyinitEventbehavior formouseover/mouseouthandlers.packages/binding.core/src/event.ts— drop legacyevent.cancelBubble = true(readonly in happy-dom, redundant tostopPropagation()).packages/provider.component/src/ComponentProvider.ts—Object.prototype.toString.call(node)instead of'' + nodeforHTMLUnknownElementdetection (immune to user-land toString overrides).packages/binding.template/helpers/dummyTemplateEngine.ts— harden templateeval(script)against bundler-renamed closure vars by passing dependencies explicitly vianew Function(...).Documented env gaps
Each skip has a one-line comment on the happy-dom-specific limitation:
attrbinding:Element.lookupNamespaceURInot implementedoptions/valuebindings (4 tests):<select>auto-selection /selectedIndexsemantics divergeeventbinding (already fixed above, no skip needed)foreachfocus (4 tests):focus()/activeElementacross DOM reorders. The 5th test in the same block ("does not preserve the target on apply bindings") is not skipped — it passes in happy-dom and still runs.defaultLoader<textarea>(2 tests): textarea content parsing differsonError(2 tests):setTimeouterrors bypasswindow.onerrorin happy-domselectExtensionsloose equality:selectedattribute via innerHTMLKnown not-addressed-here
The config in this branch also defines a
cli-bunproject, but it's knowingly incomplete — it runsALL_SPECSagainst a node environment with no setup file, which fails broadly. Correctly configuring it needs a per-package declaration of runtime support (e.g."tko": { "runtime": "dom" | "universal" }in eachpackage.json) so the test runner can include only DOM-free packages incli-bun. That's a separate follow-up branch.Test plan
bun run build && bunx vitest run --project browser— 2698 passed / 42 skipped / 0 failedbunx vitest run --project cli-happy-dom— 2677 passed / 63 skipped / 0 failedmouseover/mouseouthandlers readingevent.relatedTarget— behavior preserved🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
Chores