Visitar URL original
test: migrate hover, drag and click snapshot tests to mocks by twotnguyen · Pull Request #3192 · ChromeDevTools/chrome-devtools-mcp · GitHub
Skip to content

test: migrate hover, drag and click snapshot tests to mocks - #3192

Merged
OrKoN merged 4 commits into
ChromeDevTools:mainfrom
twotnguyen:test/migrate-hover-and-drag-tests-to-mocks
Oct 19, 2026
Merged

OrKoN merged 4 commits into
ChromeDevTools:mainfrom
twotnguyen:test/migrate-hover-and-drag-tests-to-mocks

Conversation

@twotnguyen

@twotnguyen twotnguyen commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Part of #2639.

The hover, drag, and click snapshot handler tests launched a real browser only to verify that element handles and locators are invoked and that the expected response lines and snapshot options are recorded. This migrates the unit-level checks to createHandlerMocks, following AGENTS.md, while retaining drags one element onto another as a real-browser integration test for runtime sequence coverage:

  • click › does not include snapshot by default & includes snapshot if includeSnapshot is true: asserts page.getElementByUid and locator.click with count and abort signal.
  • hover › hovers: asserts calledOnceWithExactly(page.getElementByUid, '1_1'), locator.hover with abort signal, and response line.
  • hover › includes snapshot if includeSnapshot is true: asserts page.getElementByUid, locator.hover with abort signal, and response.includeSnapshot.
  • drags › calls drag and drop on the element handles and includes snapshot: asserts calledTwice on page.getElementByUid, fromHandle.drag, toHandle.drop, response line, and response.includeSnapshot.

Testing

  • npm run build && NODE_TEST_REPORTER=spec node scripts/test.js tests/tools/input.test.ts --test-name-pattern="hovers|calls drag and drop|does not include snapshot by default": all 5 migrated mock tests pass in ~300ms.
  • Passes without a Chrome binary, running purely on mocks.
  • Retained real-browser drags one element onto another test passes cleanly with Chrome.
  • All 55 tests in tests/tools/input.test.ts pass cleanly.
  • eslint and prettier pass with zero errors.

@OrKoN
OrKoN self-requested a review October 16, 2026 09:04

@OrKoN OrKoN left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the following changes:

Keep drags one element onto another as a real-browser test (or add one real-browser drag test) and add only the new mock-based includes snapshot if includeSnapshot is true test in tests/tools/input.test.ts. The drag -> 50 ms sleep -> drop sequence is project logic whose correctness is only observable in a real browser, and #2639 asks to retain a few high-level real-browser tests.

Nits

  1. In drags one element onto another, replace the two sinon.assert.calledWith(page.getElementByUid, ...) calls with sinon.assert.calledTwice plus sinon.assert.calledWithExactly(page.getElementByUid.firstCall, '1_1') and secondCall, '1_2', per the "verify exact arguments" rule in AGENTS.md.
  2. Use one style for the two drag assertions: sinon.assert.calledOnceWithExactly(toHandle.drop, fromHandle) mirrors the fromHandle.drag assertion and does not need calledOnceWithMatch + sinon.match.same.
  3. In the click tests, sinon.assert.calledOnceWithExactly(locator.click, {count: 1, signal: sinon.match.instanceOf(AbortSignal)}) verifies the count argument that the handler derives from dblClick; calledOnce does not.

@twotnguyen
twotnguyen force-pushed the test/migrate-hover-and-drag-tests-to-mocks branch from 5a0cb9d to 5207c79 Compare October 16, 2026 09:48
@twotnguyen

Copy link
Copy Markdown
Contributor Author

Restored the real-browser drags one element onto another test in tests/tools/input.test.ts, keeping only the new mock-based includeSnapshot test.

Updated assertions:

  • drags: asserts calledTwice and calledWithExactly for firstCall and secondCall on getElementByUid. For toHandle.drop, Puppeteer's overloaded signature types the stub against DragData, so sinon.match.same(fromHandle) is used for type-safety.
  • click: tightened both click assertions to calledOnceWithMatch(locator.click, sinon.match({count: 1, signal: sinon.match.instanceOf(AbortSignal)})) to verify the derived count.

Rebased on latest main and re-pushed in commit 5207c79.

@OrKoN
OrKoN self-requested a review October 16, 2026 09:49

@OrKoN OrKoN left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the following:

  1. In tests/tools/input.test.ts › drags › includes snapshot if includeSnapshot is true, replace sinon.assert.calledOnceWithMatch(toHandle.drop, sinon.match.same(fromHandle)) with sinon.assert.calledOnceWithExactly(toHandle.drop, fromHandle). The handler calls toHandle.drop(fromHandle) with one argument, so the exact form matches the fromHandle.drag assertion above it and the "verify exact arguments" rule in AGENTS.md.

  2. In tests/tools/input.test.ts › hover › hovers and includes snapshot if includeSnapshot is true, replace sinon.assert.calledOnce(locator.hover) with sinon.assert.calledOnceWithMatch(locator.hover, {signal: sinon.match.instanceOf(AbortSignal)}). The click tests in this PR assert the signal option, and the hover handler passes {signal} the same way.

  3. Rename drags › includes snapshot if includeSnapshot is true to describe its full scope (for example calls drag and drop on the element handles and includes snapshot). The name only mentions the snapshot, but the test now holds the only mock-based coverage of the getElementByUid → drag → drop argument forwarding.

  4. Update the PR description. After commit 2, the bullet "drags one element onto another: asserts fromHandle.drag and toHandle.drop calls" and the "all 6 migrated tests" count are stale.

@OrKoN
OrKoN self-requested a review October 16, 2026 11:37
@twotnguyen

Copy link
Copy Markdown
Contributor Author
  1. Updated tests/mocks.ts with MockElementHandle so toHandle.drop types its target as ElementHandle<Element> and changed assertion to calledOnceWithExactly(toHandle.drop, fromHandle).
  2. Updated both hover tests to assert {signal: sinon.match.instanceOf(AbortSignal)} on locator.hover.
  3. Renamed drag mock test to calls drag and drop on the element handles and includes snapshot.
  4. Updated PR description to reflect the retained real-browser drag test and accurate mock coverage.

Re-pushed in commit ac83698.

@OrKoN OrKoN left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the following:

  1. In tests/mocks.ts, remove the MockElementHandle intersection type and the handle as unknown as MockElementHandle cast in createMockElementHandle. AGENTS.md forbids the as keyword for type casting, and the cast only exists to make one assertion compile.
  2. In tests/tools/input.test.ts (drags › calls drag and drop on the element handles and includes snapshot), restore the commit-2 assertion sinon.assert.calledOnceWithMatch(toHandle.drop, sinon.match.same(fromHandle)). ElementHandle.drop has two overloads and Sinon types the stub with the last (deprecated data?: DragData) one, so sinon.match.same is the type-safe way to check the exact handle without a cast.

@twotnguyen
twotnguyen force-pushed the test/migrate-hover-and-drag-tests-to-mocks branch from ac83698 to 46b4734 Compare October 16, 2026 13:42
@twotnguyen

Copy link
Copy Markdown
Contributor Author
  1. Removed MockElementHandle and the as cast in createMockElementHandle (tests/mocks.ts is now untouched and matches main).
  2. Restored sinon.assert.calledOnceWithMatch(toHandle.drop, sinon.match.same(fromHandle)) in tests/tools/input.test.ts for the overloaded ElementHandle.drop.
  3. Rebased onto latest main (19d883c).

Re-pushed in commit 46b4734.

@OrKoN
OrKoN self-requested a review October 16, 2026 13:43
@OrKoN
OrKoN added this pull request to the merge queue Oct 19, 2026
Merged via the queue into ChromeDevTools:main with commit 992c7ef Oct 19, 2026
15 checks passed
zjdCander pushed a commit to zjdCander/chrome-devtools-mcp that referenced this pull request Oct 19, 2026
## Summary

Add a **Pre-submit checklist** section to `AGENTS.md`. Each item comes
from a recent review finding or a landed bug fix in this repository.

The checklist has five groups:

- **Scope and PR description**: one change per PR, title and commit type
match, description lists every behavior change and matches the diff,
`npm run gen` and `npm run format` ran.
- **Design**: fix in the lowest layer that owns the data, fix upstream
(Puppeteer, devtools-frontend) first, one helper for repeated code,
options objects instead of positional `undefined` arguments, validation
in Zod schemas.
- **Correctness and edge cases**: no stale state after a failed call,
artifacts written before a throw, abandoned async continuations, mutex
guards, file removal races, `dispose()` releases every resource, secrets
never reach logs, path parameters validated even when they parse as
URLs.
- **Tests**: every new function has a test, no vacuous assertions,
`notCalled` targets the first call after validation, exact-argument
assertions, reusable stubs in `tests/mocks.ts`, bounded timeouts, no
second CDP session for event waits.
- **Documentation and output**: tool descriptions state when a parameter
is ignored, skills stay generic, one-sentence inline comments.

Sources: review comments on ChromeDevTools#2679, ChromeDevTools#2797, ChromeDevTools#2859, ChromeDevTools#2881, ChromeDevTools#2901, ChromeDevTools#3048,
ChromeDevTools#3082, ChromeDevTools#3109, ChromeDevTools#3120, ChromeDevTools#3132, ChromeDevTools#3134, ChromeDevTools#3139, ChromeDevTools#3148, ChromeDevTools#3156, ChromeDevTools#3158, ChromeDevTools#3161,
ChromeDevTools#3182, ChromeDevTools#3183, ChromeDevTools#3186, ChromeDevTools#3187, ChromeDevTools#3188, ChromeDevTools#3189, ChromeDevTools#3192, ChromeDevTools#3196, ChromeDevTools#3197, ChromeDevTools#3198,
ChromeDevTools#3203, ChromeDevTools#3204, and fixes ChromeDevTools#3150, ChromeDevTools#3160, ChromeDevTools#3185, ChromeDevTools#3202, ChromeDevTools#3207, ChromeDevTools#3208.

Existing content in `AGENTS.md` is unchanged.

Closes ChromeDevTools#3222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants