Add Site Management UI integration tests#3950
Conversation
There was a problem hiding this comment.
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.
|
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. 👍 |
…i' into gcsecsey/migrate-e2e-tests-to-cli
|
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! |
fredrikekelund
left a comment
There was a problem hiding this comment.
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?
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. These tests take the UI part of the CLI/UI split we agreed on in the huddle: they mount the real components and
SiteDetailsProvider, mock only the IPC bridge, and assert the commands each user action generates. The CLI integration tests in #3947 cover the parameter variations and on-disk effects.studio site duplicatecommand for the CLI suite to test.Testing Instructions
npm run typecheckand confirm there are no errorsRegression 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:
sites.test.ts)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