feat(m4-12): boa→VM PR4a — vm::test_helpers dedup#76
Merged
Conversation
Add a new `vm::test_helpers` module (engine feature, `#[doc(hidden)] pub`) hosting the four helpers that several `vm::tests::*` modules and `benches/event_dispatch.rs` currently duplicate: - `bind_vm` — unsafe thin wrapper around `install_host_data` + `Vm::bind` - `setup_with_element` — bind + element wrapper + `globalThis.el` - `make_event` — minimal `DispatchEvent` builder - `listeners_on` — owned `EventListeners` snapshot Published `#[doc(hidden)] pub` under `#[cfg(feature = "engine")]` so the bench crate (a separate compilation unit that cannot see `#[cfg(test)]` items) can reuse them without introducing a new feature flag. This follows the same pattern PR3.6 applied to `Vm::create_element_wrapper`. No call-site migrations yet; C2/C3 follow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Drop the per-file copies of `bind_vm` / `setup_with_element` / `make_event` / `listeners_on` from the four PR3-era test files that first introduced the duplication (the /simplify S4 skip item from PR3.6), and import them from `vm::test_helpers` instead. - tests_create_event_object.rs: drop local `bind_vm` + `make_event` - tests_element_wrapper.rs: drop local `bind_vm` - tests_add_event_listener.rs: drop local `setup_with_element` + `listeners_on`; call sites now pass `"div"` explicitly to match the helper's new `tag: &str` parameter - tests_remove_event_listener.rs: same as above 1767 engine / 1689 non-engine tests still pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extend the dedup to files that hand-rolled the `install_host_data` +
`vm.bind(...)` pattern inline (rather than via a local helper) and to
the event-dispatch bench:
- tests_document_global.rs: 3 inline bind blocks → `bind_vm`; inline
`EventListeners` match → `listeners_on`. The third test's
re-bind-without-reinstall remains inline (it intentionally reuses
the pre-existing HostData to verify wrapper identity across
bind/unbind cycles)
- tests_gc_audit.rs: drop local `fn bind()`, replace 5 call sites with
`unsafe { bind_vm(...) }`
- tests_promise_rejection_event.rs: keep the `bound_vm(vm, session,
dom)` convenience wrapper (unique `doc = create_document_root()` +
bind pattern repeated across 10 tests) but implement it via
`test_helpers::bind_vm` so the `install_host_data` duplication is
gone
- benches/event_dispatch.rs: import `make_event` from test_helpers;
the `Box<>`-based `setup()` stays bench-local because only the
bench needs stable addresses for the raw pointers
Clippy error count drops from 37 → 23 (baseline established in PR3.6
handoff §8 step 4); bench perf (Mouse 55ns, Keyboard 75ns, None 55ns)
unchanged.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Introduces a shared vm::test_helpers module (behind feature = "engine") to centralize repeated VM bind/setup and event/listener helper logic, then migrates existing VM engine tests and the event dispatch benchmark to use it.
Changes:
- Added
vm::test_helperswith shared helpers (bind_vm,setup_with_element,make_event,listeners_on) and exposed it as#[doc(hidden)] pubunderengine. - Updated multiple VM engine test modules to remove per-file helper duplication and import from
vm::test_helpers. - Updated
benches/event_dispatch.rsto reuse the sharedmake_eventhelper.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/script/elidex-js/src/vm/test_helpers.rs | New shared helpers for engine-backed VM tests/benches |
| crates/script/elidex-js/src/vm/mod.rs | Exposes test_helpers module under engine feature |
| crates/script/elidex-js/src/vm/tests/tests_add_event_listener.rs | Migrates to shared helpers for setup/listener inspection |
| crates/script/elidex-js/src/vm/tests/tests_remove_event_listener.rs | Migrates to shared helpers for setup/listener inspection |
| crates/script/elidex-js/src/vm/tests/tests_create_event_object.rs | Migrates to shared bind_vm + make_event helpers |
| crates/script/elidex-js/src/vm/tests/tests_element_wrapper.rs | Migrates to shared bind_vm helper |
| crates/script/elidex-js/src/vm/tests/tests_document_global.rs | Migrates to shared bind_vm + listeners_on helpers |
| crates/script/elidex-js/src/vm/tests/tests_gc_audit.rs | Migrates to shared bind_vm helper |
| crates/script/elidex-js/src/vm/tests/tests_promise_rejection_event.rs | Migrates to shared bind_vm helper via local wrapper |
| crates/script/elidex-js/benches/event_dispatch.rs | Reuses shared make_event helper (keeps bench-specific setup) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged `setup_with_element` as unsound: a safe `pub fn`
that performs an `unsafe Vm::bind` through raw pointers lets a
downstream caller trigger UB from safe code (e.g. continuing to use
`session`/`dom` while the VM still holds raw pointers into them).
Two related findings, addressed together:
1. Make `setup_with_element` `pub unsafe fn` with an explicit
`# Safety` section describing the (inherited) lifetime / aliasing
contract. Seventeen call sites in the listener-integration tests
gain `#[allow(unsafe_code)] let el = unsafe { ... };` wrappers,
matching the existing style used for `bind_vm`.
2. Delegate the `install_host_data` + `Vm::bind` sequence to
`bind_vm` instead of duplicating it. Any future change to the
bind protocol (ordering, HostData construction) now lives in one
place.
Engine tests 1767 pass, clippy `-D warnings --all-targets --features
engine` drops 23 → 21 (two redundant `as *mut _` casts vanish with
the delegation).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged `bind_vm` as unsafe to call twice on the same `Vm`:
`Vm::install_host_data` panics when a `HostData` is already installed,
so a second invocation (as arises in bind → unbind → bind cycle tests
such as `document_identity_is_stable_across_rebinds`) would abort.
Address by making the helper idempotent: only install `HostData` when
absent. Fresh VMs behave as before (install + bind); previously
unbound VMs reuse their `HostData` (`wrapper_cache` / `listener_store`
carry through), which is exactly what the identity-across-rebinds
test semantics require.
Also fold the one remaining inline rebind in
`tests_document_global::document_identity_is_stable_across_rebinds`
into `bind_vm` now that the helper handles the rebind case cleanly —
this was the last inline `unsafe { vm.bind(...) }` pattern left in
that module after C3.
Clippy `-D warnings --all-targets --features engine` drops 21 → 19
(two more redundant `as *mut _` casts vanish with the dedup).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two follow-ups from the second Copilot pass:
1. `tests_promise_rejection_event::bound_vm` was a safe wrapper around
`test_helpers::bind_vm` (an `unsafe fn`), defeating the soundness
work from r1. Promote it to `unsafe fn` with a `# Safety` section
and wrap the eleven call sites in `#[allow(unsafe_code)] unsafe {
... }`, matching the style used everywhere else after r1. Even
though `bound_vm` is module-private, Rust idiom says an `unsafe fn`
wrapped in a safe `fn` silently re-hides the contract — the
compiler's `unused_unsafe` path can only catch the delegation
when the wrapper is itself `unsafe`.
2. `tests_document_global::document_inherits_event_target_methods`
had a variable named `entries` that actually held the
`.len()` result (a `usize`) after an earlier cleanup flattened
the call chain. Rename to `entry_count` to match the value.
Engine tests 1767 / non-engine 1689 pass; clippy errors unchanged at 19.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged `listeners_on(&EcsDom, ...)` as an aliasing contract
violation: `HostData::bind` explicitly requires that no Rust reference
to `dom` exist while the VM holds its raw `*mut` pointer, yet every
`listeners_on(&dom, ...)` call site created a `&EcsDom` mid-bind.
Stacked Borrows treats that as UB, independent of whether the VM
happens to touch the pointer during the overlap.
Fix: change the helper to route through the bound pointer.
pub fn listeners_on(vm: &mut Vm, entity: Entity) -> EventListeners {
let dom = vm
.host_data()
.expect("listeners_on: HostData must be installed ...")
.dom();
...
}
Now the borrow of `EcsDom` comes from the same `HostData` that owns
the raw pointer, so no parallel Rust reference exists. Eighteen call
sites across three test modules are updated to pass `&mut vm` instead
of `&dom`. Docs on the helper spell out the soundness rationale so
the next person to extend it doesn't regress.
Engine tests 1767 pass, non-engine 1689 pass, clippy unchanged at 19.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
PR4 前座 (handoff
m4-12-pr4-handoff.md§2 PR4a) — /simplify skip item S4 (from PR3.6) 清算。vm::test_helpersmodule (engine feature,#[doc(hidden)] pub) —bind_vm/setup_with_element/make_event/listeners_onを集約成果
--features engine --all-targets)create_event_objectmise run ciClippy 14 件減は
session as *mut _/dom as *mut _pattern が 8 test files で重複していたのが 1 箇所 (test_helpers) に統合されたため。コミット構成 (3 commits)
0c44e96):vm::test_helpersmodule 新規 +vm/mod.rsmod 登録98d14b2): 必須 4 test files 移行 (tests_create_event_object/tests_element_wrapper/tests_add_event_listener/tests_remove_event_listener) — per-file helper 削除 + importe00561f): 推奨 3 test files (tests_document_global/tests_gc_audit/tests_promise_rejection_event) の inline bind pattern →bind_vmhelper、bench でmake_eventimport設計メモ
vm/tests/common.rs案 (handoff §2 原案) は#[cfg(test)] mod tests;で bench から到達不能のため回避。代わりにvm/test_helpers.rs(tests/ 外) を#[cfg(feature = "engine")] #[doc(hidden)] pub modとして配置 — PR3.6 でVm::create_element_wrapperに適用済みの同じ patternsetup_with_elementにtag: &str追加: 既存は "div" 固定だったが、PR4b でbutton/input/a等の setup が増える想定で tag 引数化。既存 call site は全て"div"を明示setup()は据え置き:Box<Vm>/Box<SessionCore>/Box<EcsDom>の stable-address pattern は stack-based test helper と不整合。集約するのはmake_eventのみtests_add_event_listener.rsのcalls_after_unbind_are_silent_no_opは install_host_data 無しの rebind 動作を意図的に検証しているため bind_vm に置換せず元の形を維持次アクション
PR4a merge 後 → PR4b (Document + Window + Location + History + Navigator) 着手。
Test plan
cargo test -p elidex-js --features engine --lib→ 1767 passcargo test -p elidex-js --lib→ 1689 passcargo bench -p elidex-js --features engine --bench event_dispatch -- --quick→ Mouse ~56ns / Keyboard ~75ns / None ~55ns 維持cargo clippy -p elidex-js --features engine --all-targets -- -D warnings→ 23 件 (baseline 37 から 14 件減)mise run ci→ fmt + clippy + test-all + deny 全 pass (local)🤖 Generated with Claude Code