Skip to content

chore: Add integration tests for CLI args to form fields #643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

richardkmichael
Copy link
Collaborator

@richardkmichael richardkmichael commented Jul 25, 2025

Add Playwright tests to cover command line argument parsing into form fields.

Motivation and Context

Command line arguments were not populating form fields as expected, which was fixed by #641. Add tests to prevent another regression.

How Has This Been Tested?

Running tests with npm run test:e2e will execute these new tests.

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@richardkmichael richardkmichael force-pushed the command-line-args-to-ui-tests branch from d4f46ef to 57973ab Compare July 25, 2025 05:28
@richardkmichael richardkmichael marked this pull request as ready for review July 25, 2025 05:32
@richardkmichael richardkmichael force-pushed the command-line-args-to-ui-tests branch from 57973ab to 3947fa6 Compare July 26, 2025 20:47
@richardkmichael richardkmichael marked this pull request as draft July 27, 2025 04:23
@richardkmichael richardkmichael force-pushed the command-line-args-to-ui-tests branch 4 times, most recently from 330b67a to b86d5fd Compare July 31, 2025 22:12
@richardkmichael richardkmichael marked this pull request as ready for review August 2, 2025 03:28
@olaservo
Copy link
Member

olaservo commented Aug 7, 2025

Thanks for adding these! I think we're good to merge these if you want to remove the extra comments.

@richardkmichael richardkmichael force-pushed the command-line-args-to-ui-tests branch from e8f464e to e4dc087 Compare August 12, 2025 22:41
Ensures Playwright itself has the configuration needed for the
Inspector.  Also allows future env without additional clutter in
`package.json`.
@richardkmichael richardkmichael force-pushed the command-line-args-to-ui-tests branch from e4dc087 to 31f2a8b Compare August 12, 2025 22:42
@richardkmichael richardkmichael marked this pull request as draft August 12, 2025 22:42
Use different server startup commands in Playwright to allow tests that
command line arguments initialize form inputs as intended.

A full integration test is used because the Inspector comprises multiple
components (package bin entry point, start command, webClient, server),
making benefit to test-effort higher, and less dependent on
implementation details.

The webServer start command uses the package entry point to ensure tests
follow the exact user experience provided by: `npx @m/inspector [...]`

Playwright's `webServer.command` cannot be [re]configured after it has
started, so multiple Playwright invocations are necessary.  To do this,
use an environment variable and a command-generating function (instead
of multiple config files).

Test tagging (`@cli`) is used to select which tests execute, while
keeping the entire test suite together.

Future:

Playwright has a feature under consideration to permit a web server per
project, which might simplify the config:
microsoft/playwright#22496
- Remove `@cli` tag, otherwise these tests will run twice, once with
  each scenario.  Untagged, they run as part of the main e2e tests.

- Use the `APP_URL`, consistent with other e2e tests.
@richardkmichael richardkmichael force-pushed the command-line-args-to-ui-tests branch from 31f2a8b to 8f05dfb Compare August 13, 2025 03:25
@richardkmichael
Copy link
Collaborator Author

This is ready, but I'm uncertain how to proceed, and how much value is here or how much effort
should be put into testing the Inspector.

This PR aims to cover the user-experience -- npx @mcp/i [args] with high testing ROI. (Otherwise,
much separate piece-wise coverage is needed; at least, for parsing of args in both start.js and
cli.ts, several function calls and invocation of subprocess with correct arguments.)

A simple testing approach is used: two scenarios, with different CLI options, result in identical UI
config, hence the same Playwright tests can run in each scenario (i.e., Command input = npm,
etc.).

In this PR as env CLI_TEST_SCENARIO:

  switch (process.env.CLI_TEST_SCENARIO) {
    case "file":
      return "npx . --config client/e2e/fixtures/everything-server.json --server everything";
    case "inline":
      return "npx . -e FOO=bar -e BAZ=bat npm --silent --prefix /path/to/mcp/servers/src/everything run start";
    default:
      return "npm run dev";
  }

controlled from client/package.json:

  "test:e2e:cli-file": "CLI_TEST_SCENARIO=file playwright test --grep @cli e2e",
  "test:e2e:cli-inline": "CLI_TEST_SCENARIO=inline playwright test --grep @cli e2e",

Combinations of new and mutually exclusive CLI options need a new Playwright web server startup command(s). Also, when command line options result in different UI state, then additional test tags would be added to run only the tests for that state.

When more command line options are added (recently, --transport and --server-url), the approach here could be unwieldy. It seems the Inspector is (helpfully) keeping pace with Claude Code for MCP-related options, so I do expect more CLI options will be added here as mcp.json and related config continue developing.

In addition, it looks like the existing CLI tests are not testing CLI parsing, but rather query param handling (via overrides in configUtils.ts), but I could be missing something.

Query param handling tests overlap conceptually with the existing default UI values (i.e., no query params) tests in transport-type-dropdown.spec.ts and should perhaps be moved there. For example, this test is nearly this test (just needs one .expect() to validate the select default).

A few options:

  1. Close
  2. Merge as is, perhaps has some value
  3. Merge after changes:
  • Add one --transport ... --server-url ... variant to the inline case
  • Move query param parsing to the other spec file (?)
  1. Write tests around start.js and cli.ts parsing and runWebClient() to cover a few of the entry pieces.
  2. ?

If not 1 or 2, maintainer feedback would be appreciated. :) Is this worthwhile? How much should
Playwright be used; and revisit #582 ?

@richardkmichael richardkmichael marked this pull request as ready for review August 13, 2025 03:46
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