-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: experimentalRunAllSpecs for component testing #32926
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
base: develop
Are you sure you want to change the base?
feat: experimentalRunAllSpecs for component testing #32926
Conversation
|
|
@scottohara I'll enable the tests to run, so keep an eye out on the results if anything needs updating. I'm not quite remembering the blocker for this originally and whether all those blockers are addressed here. It does seem like there should be more test coverage here somehow, but I'll see if someone from our team has ideas on that. |
| if (spec.relative === RUN_ALL_SPECS_KEY) { | ||
| const specsToCompile = ctx.project.runAllSpecs.map((relPath) => { | ||
| return ctx.project.specs.find((s) => s.relative === relPath) | ||
| }).filter(Boolean) as Cypress.Spec[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Robustly Handle Undefined Data Collections
Calling .map() on ctx.project.runAllSpecs without checking if it's defined will throw a TypeError if runAllSpecs is undefined. The code should use (ctx.project.runAllSpecs || []) to safely handle the undefined case, similar to how it's handled in HtmlDataSource.ts line 123.
…636-run-all-specs-for-component-testing
AtofStryker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @scottohara. Sorry for the long time on the turn around for a review here. I wanted to make sure I had time to give a thorough review. The changes look great to me but a couple things in comments below. We also need to address a few other things:
- this config / system-test needs to be updated as it is now a valid component config
- this config / system-test needs to be updated as it is now a valid root config
- We also likely want to update the
run-all-specssystem-test here as the option now pertains to component testing. We also need to add component tests to this project to make sure the feature/experiment correctly - We also should update this integration test
- I noticed some TypeScript issues that I have documented below in the PR. I think the
cypress.d.tsjust needs to moveexperimentalRunAllSpecsto the root config options. - doesn't look like the copy for
experimentalRunAllSpecsneeds to be updated
- The experiment needs to be moved out of e2e only on the documentation site
- https://github.com/cypress-io/cypress-documentation/blob/main/docs/app/references/configuration.mdx?plain=1#L201
- https://github.com/cypress-io/cypress-documentation/blob/main/docs/app/references/experiments.mdx?plain=1#L268
- https://github.com/cypress-io/cypress-documentation/blob/main/docs/app/core-concepts/open-mode.mdx?plain=1#L85
@scottohara let me know if you are able to accomplish these changes or if you need a hand in getting this across the finish line. The documentation / system-test updating isn't always trivial especially in an open source contributor context.
| cy.get('h1').contains('Choose a browser') | ||
| }) | ||
|
|
||
| it('is not a valid config for component testing', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of removing these tests, we should be checking that the experimentalRunAllSpecs option applies and works correctly for both e1e and component testing
|
Thanks for the positive comments @AtofStryker . Glad to hear that I'm on the right path at least. I'll look to address your comments and suggestions as soon as I can find some time. That said, I would also welcome anyone who wants to jump in and assist in getting this feature over the line. |
Of course, @scottohara! Sorry it took me a few weeks to get a quality review in.
If needed, I can jump in and take care of some of the items I mentioned. Just let us know! I think the only item that isn't entirely clear to me is #32926 (comment) |
|
Thanks @AtofStryker , that would be greatly appreciated if you have time to assist. |
|
I am starting to look into resolve a few of these items today |
…636-run-all-specs-for-component-testing
…636-run-all-specs-for-component-testing
…ew projects, run-all-specs-ct-vite and run-all-specs-ct-webpack to test experimentalRunAllSpecs for component testing
…636-run-all-specs-for-component-testing
|
I've gone ahead and updated the documentation for this PR in cypress-io/cypress-documentation#6342. I updated the types and a few other things, as well as what we call cy-in-cy tests, which allow us to use Cypress to test Cypress. Long story short, it gives us end to end test coverage of testing |
Additional details
This change extends the existing
experimentalRunAllSpecsconfig option to support component testing as well as e2e testing, allowing a full suite of component tests to be executed in the Cypress UI with a single click.By reusing most of the existing
experimentalRunAllSpecsmachinery, many of files touched by this PR are simply just to relax the previous restriction on it being for e2e testing only.With this PR,
experimentalRunAllSpecscan now be configured at either root level to enable it for both testing types:...or individually for each testing type:
Description of files changed
packages/app/src/store/run-all-specs-store.ts- removed checks on the testing type, allowing both e2e and componentpackages/config/src/options.ts- removes the option from thebreakingRootOptionsandtestingTypeBreakingOptionsarrays, allowing configuration at any levelpackages/data-context/schemas/schema.graphql- removed the now redundantEXPERIMENTAL_RUN_ALL_SPECS_E2E_ONLYenumpackages/errors/src/errors.ts- removed the now redundantEXPERIMENTAL_RUN_ALL_SPECS_E2E_ONLYerrorpackages/errors/test/visualSnapshotErrors.spec.ts- removes the now redundantEXPERIMENTAL_RUN_ALL_SPECS_E2E_ONLYerrorpackages/launchpad/cypress/e2e/config-warning.cy.ts- removed warnings relating to where the option config is allowedWith the above changes, the "Run [n] specs" buttons should now appear in the UI for component testing when the option is configured.
The next set of changes handle how the set of all specs is provided to either
webpack-dev-serverorvite-dev-server(depending on the bundler used).Starting with webpack:
packages/server/lib/socket-base.ts- was taught to handle theRUN_ALL_SPECS_KEY("__all"), and pass a set of specs to the CT dev server instead of a single specnpm/webpack-dev-server/src/loader.ts- was also taught to handle?specPath=__all, allowing all specs to be loadedVite was a bit tricker, and to be honest I'm not entirely happy with the implementation, so I would appreciate any feedback on a more robust and less hacky way to do this. For now:
packages/data-context/src/sources/HtmlDataSource.ts- sets a newwindow.__RUN_ALL_SPECS__property with the list of all specs to runnpm/vite-dev-server/client/initCypressTests.js- reads the above list of specs and adds each to the list of imports to load into the dev serverI'm not a regular Cypress contributor, so please do let me know if I have missed anything, if there are any additional tests needed for the above changes, or any improvements can be suggested.
Steps to test
cypress.config.(js|ts)withexperimentalRunAllSpecs: trueat either the root level or thecomponentlevelHow has the user experience changed?
PR Tasks
cypress-documentation? chore: add documentation for run all specs for component testing cypress-documentation#6342type definitions?Note
Enables
experimentalRunAllSpecsfor component testing, wiring Vite/Webpack and the runner to load/compile all specs and removing the prior e2e-only restriction and related errors.experimentalRunAllSpecsfor component (and e2e); permit config at root or per testing type. Updaterun-all-specsstore to not gate on testing type.client/initCypressTests.jsimports all specs whenspecPath="__all"usingwindow.__RUN_ALL_SPECS__; comprehensive tests added innpm/vite-dev-server/test/....src/loader.tsshouldLoad()recognizes?specPath=__all.socket-base.tscompiles multiple specs when receivingRUN_ALL_SPECS_KEY.window.__RUN_ALL_SPECS__inHtmlDataSource.experimentalRunAllSpecsinpackages/config/src/options.ts.EXPERIMENTAL_RUN_ALL_SPECS_E2E_ONLYenum and error; remove related tests and launchpad warnings.cli/CHANGELOG.mdto announce CT support forexperimentalRunAllSpecs.Written by Cursor Bugbot for commit 458f3cf. This will update automatically on new commits. Configure here.