Skip to content

Add Site Management UI integration tests#3950

Open
gcsecsey wants to merge 9 commits into
trunkfrom
gcsecsey/migrate-e2e-tests-to-cli
Open

Add Site Management UI integration tests#3950
gcsecsey wants to merge 9 commits into
trunkfrom
gcsecsey/migrate-e2e-tests-to-cli

Conversation

@gcsecsey

@gcsecsey gcsecsey commented Jun 24, 2026

Copy link
Copy Markdown
Member

Related issues

How AI was used in this PR

Claude audited the existing renderer tests to find coverage gaps, and wrote the tests.

Proposed Changes

The Site Management e2e tests drive every action through the UI, which is slow, flaky, and couples coverage to the current renderer. As we discussed on the huddle, durable logic coverage will live in the CLI integration suite (#3947), which is UI-independent.

This PR lands a small set of UI IPC-mock tests as reference examples for the command-boundary pattern on the current renderer. These tests mount the real components and SiteDetailsProvider, mock only the IPC bridge, and assert the commands each user action generates.

  • Covers all nine Site Management smoke-sheet rows, plus custom domain and HTTPS. The tests now only cover duplicate, delete, rename, and PHP version.
  • Adds coverage for site duplication, since there is no studio site duplicate command for the CLI suite to test.
  • The delete cases now run on every platform (the old e2e versions were skipped on Windows).
  • These supersede the hook-boundary assertions in the sibling unit tests, which can be retired in a follow-up.

The existing real e2e suite (apps/studio/e2e/sites.test.ts) is untouched and keeps running on every OS for true app→CLI and cross-platform coverage. These will be retired only once the new UI and the CLI round-trips are green.

Testing Instructions

  • Run the new tests:
npm test -- src/components/tests/content-tab-settings.actions.test.tsx src/modules/site-settings/tests/edit-site-details.actions.test.tsx
  • Confirm all tests pass
  • Run npm run typecheck and confirm there are no errors

Regression validation

I confirmed that these tests fail on a real regression and that they catch the same bug as the existing e2e suite. I made this change ran both suites:

diff --git a/apps/studio/src/hooks/use-delete-site.ts b/apps/studio/src/hooks/use-delete-site.ts
index 7cc117a1c..33055caa8 100644
--- a/apps/studio/src/hooks/use-delete-site.ts
+++ b/apps/studio/src/hooks/use-delete-site.ts
@@ -33,7 +33,7 @@ export function useDeleteSite() {

                if ( response === DELETE_BUTTON_INDEX ) {
                        try {
-                               await deleteSite( siteId, checkboxChecked );
+                               await deleteSite( siteId, ! checkboxChecked );
                        } catch ( error ) {
                                ipcApi.showErrorMessageBox( {
                                        title: __( 'Deletion failed' ),
EOF
Suite Bug introduced (inverted delete-files flag)
Old e2e (sites.test.ts) CleanShot 2026-06-25 at 11 28 34@2x
New UI integration tests CleanShot 2026-06-25 at 11 10 14@2x

The existing unit tests mock the hook→IPC translation layer, so these aren't catching the introduced issue, only the existing e2e tests and the newly added integration tests are.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@gcsecsey gcsecsey changed the title Add Site Management UI integration tests (STU-1867) Add Site Management UI integration tests Jun 24, 2026
@gcsecsey gcsecsey requested review from Copilot and gavande1 June 24, 2026 21:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds fast jsdom-based UI integration tests for Studio’s Site Management flows by mounting real renderer components/providers and asserting the IPC commands emitted for each user action (supporting the CLI/UI split testing strategy for STU-1867).

Changes:

  • Added UI action tests for creating sites (suggested/custom name, default/custom path, custom domain + HTTPS) asserting getIpcApi().createSite(...) calls.
  • Added UI action tests for editing site details (rename + PHP version change) asserting getIpcApi().updateSite(...) calls.
  • Added UI action tests for duplicate and delete flows asserting getIpcApi().copySite(...) / getIpcApi().deleteSite(...) calls.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
apps/studio/src/modules/site-settings/tests/edit-site-details.actions.test.tsx New integration-style tests asserting IPC calls for edit-site save + PHP version change.
apps/studio/src/modules/add-site/tests/create-site.actions.test.tsx New integration-style tests asserting IPC calls for multiple site creation variants.
apps/studio/src/components/tests/content-tab-settings.actions.test.tsx New integration-style tests asserting IPC calls for duplicate + delete behaviors from the settings tab.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@gavande1

gavande1 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Great work @gcsecsey, thank you for working on this. I like the overall approach and direction.

One suggestion: as a quick proof of concept, it would be nice to intentionally break something in the components and confirm these tests actually fail. That gives us confidence they catch real regressions and are not passing vacuously.

@gcsecsey

Copy link
Copy Markdown
Member Author

Great work @gcsecsey, thank you for working on this. I like the overall approach and direction.

One suggestion: as a quick proof of concept, it would be nice to intentionally break something in the components and confirm these tests actually fail. That gives us confidence they catch real regressions and are not passing vacuously.

Good suggestion, thanks! I added a section on this to the description. I could introduce an error and catch it with both the existing e2e and the new UI tests, but not the existing unit tests. 👍

@gcsecsey gcsecsey marked this pull request as ready for review June 25, 2026 11:34
@gcsecsey gcsecsey requested a review from fredrikekelund June 25, 2026 11:34
@gcsecsey

Copy link
Copy Markdown
Member Author

Hey @fredrikekelund could you also take a look at the approach in this PR and in #3947 and let us know what you think? Thanks!

@gcsecsey gcsecsey requested a review from a team June 29, 2026 11:19

@fredrikekelund fredrikekelund left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The actual test implementations here look pretty good to me. The IPC mocking is quite small, and is effectively quite similar to a nock interceptor, so I like that part.

Zooming out for a moment, though, I do wonder if it is really worth spending the time to refactor apps/studio E2E tests in this way if we expect to move to a new UI soon anyway?

The existing E2E tests are slow, but they do work well. It seems more valuable to either write similar tests for the new UI or work on actually migrating the app to the new UI.

I get that this PR in combination with #3947 is a kind of mock-end-to-end testing suite. It's an OK strategy for speedier CI runs, but not so good as to warrant replacing the existing E2E suite, IMO. We loose the true end-to-end part while spending time rewriting tests for a solution that'll most likely be scrapped soon anyway.

there is no studio site duplicate command for the CLI suite to test

Maybe worth adding a Linear issue?

@gavande1

Copy link
Copy Markdown
Contributor

Thanks @fredrikekelund. Let me respond point by point, since this builds on what we landed on in the discussions.

I do wonder if it is really worth spending the time to refactor apps/studio E2E tests in this way if we expect to move to a new UI soon anyway?
We loose the true end-to-end part while spending time rewriting tests for a solution that'll most likely be scrapped soon anyway.

The IPC mocking here is layer 2 of the three layers we agreed on, not a replacement for the real e2e. The true end-to-end part is not lost: it stays in the focused real round-trips (create, start, delete) that keep running on every OS. That is the reduced smoke suite you said gives sufficient assurance alongside the integration tests, and it is where the real app-to-CLI and cross-OS coverage, including bugs like the Windows spawn issue, lives.

The existing E2E tests are slow, but they do work well.
not so good as to warrant replacing the existing E2E suite

Agreed, and nothing here replaces it. As we said in the discussions, the old suite runs in parallel until the new UI is live, and deleting it is gated on the new harness plus the real round-trips being green first, so there is no coverage gap.

On your strongest point, you are right. The new UI is expected to land soon, so these mocked UI tests are tied to a renderer with a short runway and the payoff of expanding layer 2 on the old UI is thin. The durable value is the CLI suite in #3947, which is UI-independent, and the command-boundary approach itself carries over to the new UI. Given that, the open question for this PR is whether to merge it as a cheap consolidation or hold it and put the effort into the new UI instead.

@wojtekn could you weigh in here and share your opinion?

cc @gcsecsey

gcsecsey added a commit that referenced this pull request Jun 30, 2026
## Related issues

- Fixes STU-1868
- Builds on #3947 (the `site create` integration test + e2e harness) and
complements the UI suite in #3950

## How AI was used in this PR

I used Claude to write the test and the harness changes, and traced the
daemon connection failures to a Unix socket path length limit. I
reviewed the diff, ran the suite on nodes 22 and 24, and confirmed that
runs are isolated, with no orphan daemons/servers and no temp-dir leaks.

## Proposed Changes

We're migrating the legacy blueprint-based start/stop test cases to the
CLI integration approach. The existing `start.test.ts` / `stop.test.ts`
unit tests mock the daemon and server manager. This PR instead boots a
real WordPress server through the process-manager daemon and verifies
the live running state via `site list --format json`, so it actually
exercises the daemon -> server -> port lifecycle.

- Adds an `e2e`-tagged suite that creates a site with `--no-start`. It
runs in the slower (release/manual) suite, not on every PR.
- Updates the harness to prevent the tests affecting local sites. The
lifecycle commands talk to the process-manager daemon, and the cleanup
step `stop --all` would kill the developer's real running sites. The
harness now gives each run its own daemon home via
`STUDIO_PROCESS_MANAGER_HOME`, so runs are fully isolated and
parallel-safe. That home is deliberately kept short and under the temp
dir: the daemon's control socket is a Unix domain socket, and nesting it
under the `studio-cli-e2e-<uuid>` root path overflows macOS's 104-char
socket path limit. This surfaces as `EINVAL` on node 24 and as an
indefinite connect-retry (hang) on node 22. Windows uses a fixed named
pipe, so this isolation is macOS/Linux only.
- The harness also seeds a minimal `app.json` so the
`00-check-studio-compatibility` migration returns early. Without it,
that migration falls through to the developer's real legacy app data and
aborts the CLI, so the suite would pass on CI yet fail on a dev machine
with Studio installed. This also retroactively hardens the `site create`
test from #3947, which carries the same gap.
- Uses `--runtime sandbox` to stay hermetic. The native PHP runtime
drives the same lifecycle but downloads its ~25 MB binary into the
isolated config dir on the first run. Covering it hermetically needs CI
to provision that binary, so native-PHP coverage is a follow-up.

## Testing Instructions

1. `npm run cli:build`
2. `npm test -- --tagsFilter='e2e'
apps/cli/commands/site/tests/start-stop.e2e.test.ts`
3. Expect 2 tests green (`starts a site`, `stops a site`).
4. While it runs, confirm it does not disturb sites you have running in
Studio, and leaves no orphan processes or temp dirs afterward.

## Pre-merge Checklist

- [x] Have you checked for TypeScript, React or other console errors?
@wojtekn

wojtekn commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@fredrikekelund @gavande1 it's worth adding that we will have both UIs working in parallel for a while (and possibly allow opt-in via a beta feature or similar), so having tests for both UIs makes sense.

I think it makes sense to merge the PR, but scoped down - just a few IPC-mock tests as reference examples rather than a full port of the suite.

To avoid spending too much time on tests that will be removed at some point, it makes sense to prioritize those in the following way:

  1. Add new CLI integration tests (they will stay regardless of the UI chosen)
  2. Add new UI tests with IPC mocks for old UI (just a few, to develop good practice examples)
  3. Add/port new UI tests with IPC mocks from the old UI to the new UI
  4. Remove the old UI tests along with the old UI (alternatively, remove them as soon as we add the corresponding CLI tests mentioned in 1)

Also, to be clear, we are accepting that most of the UI-flow coverage gets dropped in favor of CLI logic coverage.

@gcsecsey

gcsecsey commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

@fredrikekelund @gavande1 it's worth adding that we will have both UIs working in parallel for a while (and possibly allow opt-in via a beta feature or similar), so having tests for both UIs makes sense.

I think it makes sense to merge the PR, but scoped down - just a few IPC-mock tests as reference examples rather than a full port of the suite.

To avoid spending too much time on tests that will be removed at some point, it makes sense to prioritize those in the following way:

  1. Add new CLI integration tests (they will stay regardless of the UI chosen)
  2. Add new UI tests with IPC mocks for old UI (just a few, to develop good practice examples)
  3. Add/port new UI tests with IPC mocks from the old UI to the new UI
  4. Remove the old UI tests along with the old UI (alternatively, remove them as soon as we add the corresponding CLI tests mentioned in 1)

Also, to be clear, we are accepting that most of the UI-flow coverage gets dropped in favor of CLI logic coverage.

I reduced the scope of this PR and updated the PR description.

@gcsecsey gcsecsey requested a review from fredrikekelund July 1, 2026 14:39
@gcsecsey gcsecsey requested a review from wojtekn July 1, 2026 14:41
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.

5 participants