Skip to content
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(testIsolation): improve the behavior, clarify config options and sync with session command #24316

Merged
merged 17 commits into from
Oct 24, 2022

Conversation

emilyrohrbough
Copy link
Member

@emilyrohrbough emilyrohrbough commented Oct 19, 2022

End to end testing

experimentalSessionAndOrigin=false

|testIsolation | beforeEach test | cy.session |
|----------------|---------------------------------------|------------------------------------|
|legacy | clear cookies & local storage in current domain | not available |

experimentalSessionAndOrigin=true

testIsolation beforeEach test cy.session
on clear page, clear cookies & local & session storage in all domains clear page, clear cookies & local & session storage in all domains
off do nothing to browser context clear cookies & local & session storage in all domains

Component Testing

No real impact on Component testing. When sessions & origin goes GA, session storage will also be cleared between tests. Currently only the page, cookies and local storage are cleared.

User facing changelog

The behavior and implementation of the experimental strict test isolation mode has been evaluated per user feedback. strict mode has been replaced test isolation on and off modes, with on mode being the default behavior observed when experimentalSessionAndOrigin=true and the cy.session() command now inherits the test isolation behavior for the suite it runs in. When enabled, legacy mode is not longer a valid mode and to persist the browser context between tests off mode can be used.

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 19, 2022

Thanks for taking the time to open a PR!

@mjhenkes mjhenkes self-requested a review October 20, 2022 14:02
cli/types/cypress.d.ts Outdated Show resolved Hide resolved
@AtofStryker AtofStryker self-requested a review October 20, 2022 14:18
Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

Looks really well tested! Main questions I have are around the comments in cypress.d.ts and wonder if we want to reflect similar language in the Docs PR?

cli/types/cypress.d.ts Outdated Show resolved Hide resolved
cli/types/cypress.d.ts Outdated Show resolved Hide resolved
cli/types/cypress.d.ts Outdated Show resolved Hide resolved
cli/types/cypress.d.ts Outdated Show resolved Hide resolved
*
* NOTES:
* - Turning test isolation off may improve performance of end-to-end tests, however, previous tests could impact the
* browser state of the next test and cause inconsistency when using .only().
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also have an impact if the user changes the order of their tests. I think what we are trying to convey is that tests are not guaranteed idempotency when run by themselves and I am not sure that is obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

This could also have an impact if the user changes the order of their tests.

This would depend on how the user has written their E2E tests -- you could write basic, repeatable tests that don't impact the state of the next test and could be ran standalone. Though most people may accidentally write tests that do rely on one another. If you / others feel strongly, I'd prefer to elaborate more in about the risks in the documentation vs the Typescript description....It might be worth removing this entire blur and adding an onlink to learn more info.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is pretty clear here with what you added. I wonder if we want to generalize this more as to your point, this is a TypeScript declaration file and not docs 😄 . Maybe something like:

Turning test isolation off may improve performance of end-to-end tests, however, previous tests may not be idempotent. @see <LINK_TO_DOCS>

which would give the clear description of what is meant here when we mention idempotent, since its probably an appropriate word to capture what we mean here, but not suitable for an in depth explanation which the docs provide.

cli/types/cypress.d.ts Outdated Show resolved Hide resolved
packages/config/src/browser.ts Show resolved Hide resolved
packages/config/src/project/utils.ts Outdated Show resolved Hide resolved
.then((win) => {
win.cookie = 'key=value; SameSite=Strict; Secure; Path=/fixtures'
win.localStorage.setItem('animal', 'bear')
win.sessionStorage.setItem('food', 'burgers')
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need a similar test check if document.cookie and the cookie-jar are cleared between tests when going back to cy.origin?

@emilyrohrbough
Copy link
Member Author

@AtofStryker I tried to match the language to the docs changes while also make it comprehensive in the "what is test isolation" blurb. I'll take a look through your comments, but if you could make notes on the docs PR as well where you feel these diverge, that'd be helpful. I've been thinking/talking about this for so long it all makes sense to me 😄

@AtofStryker
Copy link
Contributor

@AtofStryker I tried to match the language to the docs changes while also make it comprehensive in the "what is test isolation" blurb. I'll take a look through your comments, but if you could make notes on the docs PR as well where you feel these diverge, that'd be helpful. I've been thinking/talking about this for so long it all makes sense to me 😄

@emilyrohrbough after taking a look, I think the docs capture that context very well. I think some of the updates proposed to the cypress.d.ts mirror what is described in your docs PR.

cli/types/cypress.d.ts Outdated Show resolved Hide resolved
cli/types/cypress.d.ts Outdated Show resolved Hide resolved
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.

Change TestIsolation legacy to "on" / false "off" tie specifically to browser context
3 participants