Add a Playwright e2e test #460

msabramo · 2025-05-28T01:34:49Z

Write a Playwright test that tests the app end-to-end (e2e).

Motivation and Context

Ensure things don't get broken

How Has This Been Tested?

abramowi at Marcs-MacBook-Pro-3 in ~/Code/OpenSource/inspector (playwright-test●)
$ npm run test:e2e

> @modelcontextprotocol/[email protected] test:e2e
> npm run test:e2e --workspace=client


> @modelcontextprotocol/[email protected] test:e2e
> playwright test e2e


Running 8 tests using 5 workers
  8 passed (7.9s)

To open last HTML report run:

  npx playwright show-report

Breaking Changes

No

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

Additional context

msabramo · 2025-05-28T02:22:11Z

Related PR (which has perhaps fallen into some disrepair): #354

olaservo · 2025-05-30T16:03:12Z

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.

cliffhall

The test ran successfully, but only after

I downloaded the browsers (additional step required beyond just npm install)
Launching the inspector (you can't just run test:e2e, you have to run start then test:e2e)

Just thinking about how to handle this in CI. We'd need to use caching to install playwright and browsers only if they aren't cached. And start the server, then wait for it to be up before running the tests.

name: Playwright Tests

on:
  push:
    branches: [ main ] 
  pull_request:
    branches: [ main ]

jobs:
  test:
    timeout-minutes: 5
    runs-on: ubuntu-latest 

    steps:
      - uses: actions/checkout@v4

      - uses: actions/setup-node@v4
        with:
          node-version: 18 

      # Cache Playwright browsers
      - name: Cache Playwright browsers
        id: cache-playwright
        uses: actions/cache@v4 
        with:
          path: ~/.cache/ms-playwright # The default Playwright cache path
          key: ${{ runner.os }}-playwright-${{ hashFiles('package-lock.json') }} # Cache key based on OS and package-lock.json
          restore-keys: |
            ${{ runner.os }}-playwright-

      - name: Install dependencies
        run: npm ci 

      - name: Install Playwright and browsers unless cached
        run: npx playwright install --with-deps
        if: steps.cache-playwright.outputs.cache-hit != 'true' 

      - name: Start Server
        run: npm run start 

      - name: Wait for server to start
        run: npx wait-on http://localhost:6274

      - name: Run Playwright tests
        run: npm run test:e2e

package.json

msabramo · 2025-06-04T06:48:58Z

@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 · 2025-06-05T07:23:07Z

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?

cliffhall · 2025-06-05T23:21:47Z

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 · 2025-06-07T01:20:38Z

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

Done in df4c372.

so that the CI job can proceed

msabramo · 2025-06-07T15:32:08Z

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 · 2025-06-07T19:19:58Z

@olaservo @cliffhall

cliffhall · 2025-06-17T19:23:02Z

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

cliffhall · 2025-06-17T20:47:36Z

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

since now we are doing that in `playwright.config.ts`

msabramo · 2025-06-18T03:25:42Z

@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 · 2025-06-18T06:44:15Z

@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

cliffhall

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

Thanks so much for getting it to auto start the server. There were no instructions to start the server, and for a local test run, it shouldn't really have to be a requirement anyhow.

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

Please forgive the bluntness, but in an already crowded project, these folders and files are just litter to me. IMO, adding them to .gitignore and the clean script are not an acceptable solution. The former means they remain in my project, visual clutter that I do not need to see. The latter means I need to run another command, which also removes all the node modules and does another npm install, time I do not need to waste.

When running tests, I only care about whether they failed or passed, period. The following is all I need to see:

If you could simply make the test:e2e script delete these files after the run, it would be perfect.

msabramo · 2025-06-19T14:29:30Z

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

cliffhall

Thanks @msabramo for cleaning up the unnecessary remnants. One more new side effect just showed up (not your fault in the least). Working fix suggested below.

package.json

@cliffhall

Since merging the latest changes from `main` that put auto-open back in when running the start or dev scripts, the `test:e2e` run results in the Inspector being opened in the browser. This change suppresses that (tested locally). Thanks, @cliffhall! Co-authored-by: Cliff Hall <[email protected]>

cliffhall

LGTM! 👍

Need to discuss the github action

cliffhall

Let's streamline this action to just run the tests and make a comment on the PR with the results.

cliffhall · 2025-06-19T19:41:00Z

.github/workflows/e2e_tests.yml

+            client/playwright-report/
+            client/test-results/
+            client/results.json
+          retention-days: 30


If this is done on every push to main, we don't need to retain these files for 30 days.

Reduced from 30 to 2 in eb89e01.

cliffhall · 2025-06-19T19:48:08Z

.github/workflows/e2e_tests.yml

+      - name: Publish Playwright Test Summary
+        uses: daun/playwright-report-summary@v3
+        if: always()
+        with:
+          report-file: client/results.json
+          comment-title: "🎭 Playwright E2E Test Results"
+          job-summary: true
+          icon-style: "emojis"
+          custom-info: |
+            **Test Environment:** Ubuntu Latest, Node.js 18
+            **Browsers:** Chromium, Firefox
+
+            📊 [View Detailed HTML Report](https://github.com/${{ github.repository }}/actions/runs/${{ github.run_id }}) (download artifacts)
+          test-command: "npm run test:e2e"


I don't know what the intention of all this is, it seems like its running the tests twice. We've already run it above in Run Playwright tests and here we're running it to generate the report again...?

I think what we really want is the simple report with comment setup. There's no need to upload the reports and save/publish them. Just the results in a PR comment will do.

I don't know what the intention of all this is, it seems like its running the tests twice. We've already run it above in Run Playwright tests and here we're running it to generate the report again...?

I could be wrong, but my assumption is that it's not running the tests again. I think maybe:

test-command: "npm run test:e2e"

might be creating that impression? And it might be misleading because the docs for this action say:

# Command used to run tests. If provided, a command to re-run failed or # flaky tests will be printed for each section. # Default: '' test-command: 'npm run test --'

so it sounds like it be more informative and not actually a way to run the tests, as it kind of seems it could be?
But maybe this needs a comment to clarify, since that is confusing?

I think what we really want is the simple report with comment setup. There's no need to upload the reports and save/publish them. Just the results in a PR comment will do.

So I think this is set up to do that but it's failing to post the comment because of permissions problems:

Commenting test report on PR Creating new comment Error creating comment: Resource not accessible by integration Submitting PR review comment instead... Error creating PR review: Resource not accessible by integration Warning: Unable to comment on your PR — this can happen for PR's originating from a fork without write permissions. You can copy the test results directly into a comment using the markdown summary below:

Maybe you can have a look and see if you are able to grant permissions so that this can work and you can see if you like the way it does comments?

cliffhall

Try the below suggestion.

.github/workflows/e2e_tests.yml

Co-authored-by: Cliff Hall <[email protected]>

msabramo · 2025-06-20T18:32:10Z

Still got the following after applying d5b4a6e:

msabramo · 2025-06-20T18:36:41Z

msabramo · 2025-06-20T18:37:37Z

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)

cliffhall reviewed Jun 2, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

cliffhall added the waiting on submitter Waiting for the submitter to provide more info label Jun 4, 2025

msabramo added 15 commits June 6, 2025 19:33

Add a Playwright e2e test

226ad25

npm run prettier-fix

69da70a

Exclude e2e from Jest

8a13526

Make npm run test:e2e work

8babc44

Mention npm run test:e2e in CONTRIBUTING.md

3a28efe

Add more Playwright tests

89e1efc

Move test:e2e up with other two test scripts

c803d93

Add .github/workflows/e2e_tests.yml

4f6a4ce

npm run prettier-fix

66ad85a

Do npm run start in background

1df83d7

so that the CI job can proceed

Upload Playwright Report and Screenshots

826bc35

git add playwright.config.ts

a421f32

npm run prettier-fix

f06a6e1

Install Playwright dependencies

a2a58c2

e2e_tests.yml: playwright-report and test-result in client dir

705d7b4

msabramo force-pushed the playwright-test branch from e8b503d to 705d7b4 Compare June 7, 2025 02:34

Generate report with https://github.com/daun/playwright-report-summary

cdb9180

msabramo force-pushed the playwright-test branch from 990ff30 to cdb9180 Compare June 7, 2025 15:22

Take screenshots on failure

2c0af88

msabramo force-pushed the playwright-test branch from 862bd61 to 2c0af88 Compare June 7, 2025 15:36

msabramo added 2 commits June 17, 2025 20:18

playwright.config.ts: Start dev server automatically

8b1faf1

e2e_tests.yml: Remove start dev server

4ef6bdb

since now we are doing that in `playwright.config.ts`

prettier fix playwright.config.ts

0f1cc19

msabramo added 2 commits June 17, 2025 23:44

.gitignore: Add playwright artifacts

0b31ce7

Make "npm run clean" remove Playwright files

b783b50

cliffhall requested changes Jun 18, 2025

View reviewed changes

msabramo added 5 commits June 19, 2025 00:00

No playwright-report or results.json for non-CI

db9ca37

Merge branch 'main' into playwright-test

2ca63cb

outputDir: "./e2e/test-results"

08e4f6b

prettier fix

8b3613e

Cleanup client/e2e/test-results/ after test:e2e

1807542

msabramo force-pushed the playwright-test branch from 16763de to 1807542 Compare June 19, 2025 14:23

msabramo requested a review from cliffhall June 19, 2025 14:29

cliffhall requested changes Jun 19, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

cliffhall previously approved these changes Jun 19, 2025

View reviewed changes

cliffhall requested changes Jun 19, 2025

View reviewed changes

olaservo mentioned this pull request Jun 20, 2025

Add e2e tests #354

Closed

9 tasks

cliffhall requested changes Jun 20, 2025

View reviewed changes

.github/workflows/e2e_tests.yml Show resolved Hide resolved

Update .github/workflows/e2e_tests.yml

d5b4a6e

Co-authored-by: Cliff Hall <[email protected]>

Change retention-days from 30 to 2

eb89e01

Add a Playwright e2e test #460

Are you sure you want to change the base?

Add a Playwright e2e test #460

Conversation

msabramo commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

msabramo commented May 28, 2025

Uh oh!

olaservo commented May 30, 2025

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msabramo commented Jun 4, 2025

Uh oh!

msabramo commented Jun 5, 2025

Uh oh!

cliffhall commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msabramo commented Jun 7, 2025

Uh oh!

msabramo commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msabramo commented Jun 7, 2025

Uh oh!

cliffhall commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliffhall commented Jun 17, 2025

Uh oh!

msabramo commented Jun 18, 2025

Uh oh!

msabramo commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

msabramo commented Jun 19, 2025

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

cliffhall Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

msabramo Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

cliffhall Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

msabramo Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

msabramo Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msabramo commented May 28, 2025 •

edited

Loading

cliffhall commented Jun 5, 2025 •

edited

Loading

msabramo commented Jun 7, 2025 •

edited

Loading

cliffhall commented Jun 17, 2025 •

edited

Loading

msabramo commented Jun 18, 2025 •

edited

Loading

msabramo Jun 20, 2025 •

edited

Loading

msabramo commented Jun 20, 2025 •

edited

Loading