Add a Playwright e2e test

[msabramo]

Write a Playwright test that tests the app e2e.

Motivation and Context

Ensure things don't get broken

How Has This Been Tested?

abramowi at marcs-mbp-3 in ~/Code/OpenSource/inspector (playwright-test●)
$ npx playwright test client/e2e/transport-type-dropdown.spec.ts

Running 1 test using 1 worker

  ✓  1 … Type Dropdown › should have options for STDIO, SSE, and Streamable HTTP (764ms)

  1 passed (1.5s)

Breaking Changes

No

Types of changes

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

Checklist

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

Additional context

[msabramo]

Related PR (which has perhaps fallen into some disrepair): https://github.com/modelcontextprotocol/inspector/pull/354

[olaservo]

Thanks for setting this up. My draft PR I'd opened for feedback has been getting pretty moldy. A couple questions about this e2e setup:

• Are you thinking these should just be run locally during development? Originally I assumed that we should have these run as part of CI so we're not relying on that. I think it would also be OK to work that in as separate PRs.
• I'm making a similar assumption with adding Playwright configs - my original PR had some settings for browser, timeouts, etc. which we can add as we need them.
• How do you think we should approach adding tests for different transports? In my original tests I was duplicating a lot of code.

Thanks again for working on this, it should help bridge some gaps with testing basic stuff efficiently.

[msabramo]

@olaservo: I had been thinking that this would mainly be run in CI but I like to do things in baby steps so that I don't get overwhelmed and so reviewing is easier. My plan was to do the CI in a separate PR.

Though now I'm wondering if we should just go for it since @cliffhall points out that it's not trivial to set up the tests locally and a lot of people will simply not bother, so maybe it's not of much use if it's not also run in CI.

I probably should look at your PR and see what good stuff I can borrow re: config, timeouts, etc.

[msabramo]

Added .github/workflows/e2e_tests.yml (from @cliffhall) in 8a8212c9ca420f998bfa9da3f3bde2e11f23bcbb. I guess a maintainer needs to approve the workflow to run so we can see how it works?

[cliffhall]

Added .github/workflows/e2e_tests.yml (from @cliffhall) in 8a8212c. I guess a maintainer needs to approve the workflow to run so we can see how it works?

@msabramo I tried but it failed on format check. Need to run prettier-fix to pass CI.

[msabramo]

@msabramo I tried but it failed on format check. Need to run prettier-fix to pass CI.

Done in df4c372.

[msabramo]

I added support for https://github.com/daun/playwright-report-summary in cdb9180.

As a result, at https://github.com/modelcontextprotocol/inspector/actions/runs/15509171521?pr=460 you can see a nice Playwright test report:

[msabramo]

@olaservo @cliffhall

[cliffhall]

@msabramo Failures when I run the tests locally with /usr/local/bin/npm run test:e2e. What am I missing?

[cliffhall]

@msabramo Also, I noticed that running these tests created a lot of files locally in the project. Can these not be created in the system tmp folder?

I realize the CLI tests do such a thing as well, and I'd like to make that go away too, but that's for another PR.

[msabramo]

@msabramo Failures when I run the tests locally with /usr/local/bin/npm run test:e2e. What am I missing?

Well, it could probably be a few things, but one possibility is that perhaps the dev server wasn't running.

Regardless of whether that's the problem you're hitting, it seems like an easy problem to hit. I discovered that Playwright can start the server for us with a webServer config in playwright.config.ts which I added in 8b1faf1. And then I removed a similar thing in the e2e_tests.yml GH Actions workflow, because Playwright should take care of it inside and outside of CI, which seems better.

Give it another try with the new commits and see if it works any better.

If it still doesn't work, you might try opening a browser to http://localhost:6274 while the Playwright tests are running and opening a JavaScript console and looking for errors. I actually ran into another problem that confused me for a while where the server was starting but the web page was blank and it was because of a SyntaxError in the JS Console, which was resolved by running npm install to get some new package(s) that are necessary.

[msabramo]

@msabramo Also, I noticed that running these tests created a lot of files locally in the project. Can these not be created in the system tmp folder?

I realize the CLI tests do such a thing as well, and I'd like to make that go away too, but that's for another PR.

You mean these:

$ git status
On branch playwright-test
Your branch is up to date with 'msabramo/playwright-test'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        client/playwright-report/
        client/results.json
        client/test-results/

The first two are reports that are explicitly specified in playwright.config.ts, so they seem potentially useful and I'm not sure what client/test-results is for, but probably something Playwright uses, so I'd be nervous about moving them. How about if I add them to .gitignore? Does that help with your concern?

0b31ce7 - .gitignore: Add playwright artifacts

b783b50 - Make "npm run clean" remove Playwright files

[msabramo]

@cliffhall: Thanks for the detailed comments! I think it should be better now. With the latest commits, I don't even generate playwright-report or results.json if we're not running in CI (those are needed for the GitHub Action workflow) and test-results is moved to client/e2e/test-results and it gets removed by the test:e2e script at the end if we're not running in CI.

So I think things should be clean but let me know if I missed anything.

[msabramo]

Still got the following after applying d5b4a6e:

[msabramo]

[msabramo]

Here's the markdown summary from the GH Actions log, manually pasted here so we can see how it looks:

---

🎭 Playwright E2E Test Results

✅ 12 passed

Details

12 tests across 1 suite
22.3 seconds
d5b4a6e
ℹ️ Test Environment: Ubuntu Latest, Node.js 18 Browsers: Chromium, Firefox

📊 View Detailed HTML Report (download artifacts)

---

[msabramo]

I wonder if 1ff39f1 caused something to trigger that asked a maintainer to approve execution?

[cliffhall]

I wonder if 1ff39f1 caused something to trigger that asked a maintainer to approve execution?

Nope. That's why I asked to remove it. It did not help, but could potentially be a risk.

[msabramo]

I wonder if 1ff39f1 caused something to trigger that asked a maintainer to approve execution?

Nope. That's why I asked to remove it. It did not help, but could potentially be a risk.

Okay, reverted the two changes in 9ffe63e and 3e28357.

[msabramo]

Maybe once it gets merged, the comments will work on builds on main (probably not on PRs in forks, which is probably a good thing).

[cliffhall]

Maybe once it gets merged, the comments will work on builds on main (probably not on PRs in forks, which is probably a good thing).

Can you also remove the permissions block

[msabramo]

Can you also remove the permissions block

Yup, in 465ee9a.

[cliffhall]

One last question: Is there a way to get it to stop trying to make the PR comment? It's still reporting that it can't do it.

[msabramo]

One last question: Is there a way to get it to stop trying to make the PR comment? It's still reporting that it can't do it.

I think c747fe8 might do the trick. Let's see...

[msabramo]

I think c747fe8 might do the trick. Let's see...

Now it skips that step entirely for forks, which I guess is fine.

It might be possible to have it create the report and just not comment, but not sure if it's worth it...

[msabramo]

[cliffhall]

I re-ran the jobs just to see, and it does not produce the failed annotation warning now.

[cliffhall]

Thanks for hanging in there @msabramo.