-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: remove CT side effects from mount when e2e testing #22633
Conversation
Thanks for taking the time to open a PR!
|
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.
Two small comments.
npm/mount-utils/src/index.ts
Outdated
// System test to verify CT side effects do not pollute e2e: system-tests/test/e2e_with_mount_import_spec.ts | ||
if (Cypress.testingType !== 'component') { | ||
// eslint-disable-next-line no-console | ||
console.log('Skipping component setup as Cypress is not running in component mode. You are probably seeing this message due to an inclusion of a "mount" import in your support file.') |
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.
I wonder if we even want to log this? Does a user really want to/need to see this? Eg - if you are running e2e tests with the console open, you'll see this a lot.
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.
Gonna remove it. We should probably outright block the use of mount
in e2e tests but that was outside of the scope of this PR so I didn't tweak it.
@@ -0,0 +1,5 @@ | |||
module.exports = { | |||
e2e: { |
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 adding a new project, couldn't we use an existing one that has CT and E2E and just update the support/e2e.js
to import cypress/react
? Just to minimize the number of example projects.
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.
Moved to component-tests
project, keeping the system-test project bloat down is good with me
@ZachJW34 I'm still seeing this error when running e2e tests with this branch. My
Am I missing something? |
@astone123 Due to how the packages are bundled with the binary, they don't get synced with a normal |
@ZachJW34 I tried running
In my project with that |
@astone123 We chatted, testing this change with a project outside of the monorepo will resolve |
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Looks good, let's get this green and ship it. |
I thought about this a bit more @ZachJW34 - the current approach might not be ideal. The fact we import at all, even if we return early, is still a side effect, since it's imported, and bundlers/preprocesses might try to do things (like analyze other imports in that module). Example: In #22437 we do a dynamic import for In the This is going to be a problem, and more confusing that just raising an error. I'll sync up w/ @ZachJW34 on this, but I think we should probably just ask users to be disciplined about separating E2E and CT commands. We can't really predict what bundles/preprocessors will do, so users need to assume that if something is imported *at all, side effects **are ** possible. Rather than leaving them with a puzzling bundler behavior (like the E2E test I linked to exhibits) I think throwing an error is probably better, since it's at least easy to understand. |
User facing changelog
CT side effects from
import { mount } from 'cypress/{react,vue}'
are disabled when running e2e tests.Additional details
Though we scaffold CT with the mount registration in
cypress/support/component.js
, a user might lift this registration to the scaffoldedcommands.js
file, which is not specific to testing type. There are side effects associated with the mount import that are required for CT testing but they will cause errors when e2e testing.Though we have e2e and component specific support files, users might be inclined to keep all of their custom commands in a single file as documented here. Doing so should not cause these errors to surface, so I've stubbed out the CT side effects when running e2e (and improved a few errors thrown due to not finding the root selector).
Ideally, I don't want the
mount
import to have side effects. I think there should be an explicit import/registration that would be used inside of thecomponent.js
support file, but that would be a breaking change so I've left some comments around a future refactor of this.Steps to test
The original issue has a good reproduction and I've included a system test to verify the new behavior. You can scaffold a basic project with e2e and component testing and lift the
Cypress.Commands.add('mount', ...)
registration intocommands.js
then try to run an e2e test and the error will be thrown.How has the user experience changed?
Before:
After:
PR Tasks
cypress-documentation
?type definitions
?