Skip to content

test: migrate memory tests off withMcpContext - #2729

Open
maheshsingh20 wants to merge 1 commit into
ChromeDevTools:mainfrom
maheshsingh20:test/memory-unit-tests
Open

test: migrate memory tests off withMcpContext#2729
maheshsingh20 wants to merge 1 commit into
ChromeDevTools:mainfrom
maheshsingh20:test/memory-unit-tests

Conversation

@maheshsingh20

Copy link
Copy Markdown
Contributor

Description

This PR migrates fixture-based heap snapshot tests off withMcpContext to avoid launching and closing headless Chromium unnecessarily:

  • Formatting tests relocated: Moved fixture-based formatting tests from tests/tools/memory.test.ts to tests/formatters/HeapSnapshotFormatter.test.ts using HeapSnapshotManager directly (exercises genuine .heapsnapshot fixtures in Node without browser overhead).
  • Tool handler unit tests: Converted tool handlers in tests/tools/memory.test.ts to mock-based unit tests using createHandlerMocks() to verify parameter validation, default handling, and method delegation to context and response.
  • E2E test retained: Kept take_heapsnapshot as a real-browser e2e test with withMcpContext (genuinely requires live CDP captureHeapSnapshot on a real page).
  • Caching test relocated: Moved comparison cache test to tests/processors/HeapSnapshotManager.test.ts.
  • Mock helpers: Added typed mock factories to tests/mocks.ts.

Test Plan

  • npm run build
  • node scripts/test.js tests/processors/HeapSnapshotManager.test.ts
  • node scripts/test.js tests/formatters/HeapSnapshotFormatter.test.ts
  • node scripts/test.js tests/tools/memory.test.ts
  • npm run check-format
  • npm run docs:generate

Migrate fixture-based heap snapshot tests off withMcpContext to avoid unnecessary browser launches:
- Relocate fixture-based formatting tests to HeapSnapshotFormatter tests using HeapSnapshotManager directly.
- Convert tool handler tests in memory tests to mock unit tests verifying parameter validation and delegation using createHandlerMocks().
- Retain take_heapsnapshot as an e2e test with withMcpContext.
- Relocate snapshot comparison caching test to HeapSnapshotManager tests.
@maheshsingh20

Copy link
Copy Markdown
Contributor Author

Hi @OrKoN,

This PR implements the split we discussed for migrating memory tests off withMcpContext:

  1. Fixture formatting tests relocated: Moved all fixture-based formatting tests to tests/formatters/HeapSnapshotFormatter.test.ts using HeapSnapshotManager directly (exercises genuine .heapsnapshot fixtures in Node with zero browser launches).
  2. E2E test retained: Kept take_heapsnapshot as a real-browser e2e test with withMcpContext (genuinely requires live CDP captureHeapSnapshot).
  3. Tool handler unit tests: Converted the remaining 12 tool handlers in tests/tools/memory.test.ts to fast mock unit tests using createHandlerMocks() to verify parameter parsing, default fallbacks, and delegation.
  4. Caching test relocated: Moved comparison cache test to tests/processors/HeapSnapshotManager.test.ts.

All tests pass cleanly and formatting checks are green. Could you please take a look when you have a moment? Thanks!

@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.

Let's also add the following impovements:

  1. Restore dropped error handling & lifecycle tests in HeapSnapshotManager.test.ts:
    • Add tests for getNodesById() throwing on non-existent class ID.
    • Add tests for getDetailedClassDiff() throwing on invalid classIndex.
    • Add tests for disposeSnapshot() and getStats() / getStaticData().
  2. Add unit tests for heap snapshot formatting in tests/McpResponse.test.ts:
    Test response.setHeapSnapshot*() and response.handle() with mock data in tests/McpResponse.test.ts to ensure coverage for headers, stats formatting, pagination summaries, and node sorting.
  3. Complete tool handler test cases in tests/tools/memory.test.ts:
    Add the missing default and paginated parameter test cases for get_heapsnapshot_retainers, get_heapsnapshot_duplicate_strings, get_heapsnapshot_retaining_paths, and unit test take_heapsnapshot.

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