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: allow running tests outside Vite project root folder #25801

Merged

Conversation

IlCallo
Copy link
Contributor

@IlCallo IlCallo commented Feb 14, 2023

Additional details

See explanation here: #22505 (comment)

Note that I'm not sure where @fs comes from, probably a Vite plugin?

Steps to test

You'll need Yarn 1, unluckily we're still stuck with that

  • clone https://github.com/IlCallo/quasar and checkout upgrade-cypress-ae
  • cd ui
  • yarn install
  • cd dev
  • yarn install
  • cd ..
  • yarn test:component
  • notice Cypress can correctly detect tests
  • click on any test, I did my tests on QAvatar ones
  • notice the request returns 404, due to a wrong relative path composition

How has the user experience changed?

We're now able to reference tests outside Vite project root folder in Quasar monorepo

PR Tasks

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2023

CLA assistant check
All committers have signed the CLA.

@cypress
Copy link

cypress bot commented Feb 14, 2023

41 flaky tests on run #44062 ↗︎

0 26802 1271 0 Flakiness 41

Details:

fix: allow running tests outside Vite project root folder
Project: cypress Commit: 9db5251a0e
Status: Passed Duration: 20:01 💡
Started: Feb 14, 2023 8:49 AM Ended: Feb 14, 2023 9:09 AM
Flakiness  create-from-component.cy.ts • 1 flaky test • app-e2e

View Output Video

Test
... > runs generated spec Screenshot
Flakiness  specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e

View Output Video

Test
App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs Screenshot
Flakiness  cypress-in-cypress-component.cy.ts • 1 flaky test • app-e2e

View Output Video

Test
Cypress In Cypress CT > default config > redirects to the specs list with error if a spec is not found Screenshot
Flakiness  cypress-origin-communicator.cy.ts • 1 flaky test • app-e2e

View Output Video

Test
Cypress In Cypress Origin Communicator > cy.origin passivity with app interactions > passes upon test reload mid test execution Screenshot
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test
network stubbing > intercepting request > can delay and throttle a StaticResponse

The first 5 flaky specs are shown, see all 21 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@ZachJW34
Copy link
Contributor

@IlCallo Could you run SNAPSHOT_UPDATE=1 yarn workspace @tooling/system-tests test vite_dev_server_fresh_spec (after running yarn and making sure everything is built)? It should update our snapshot file and then you can push those up in a separate commit to get the tests to pass.

Also, we have an explicit fs.allow list

so I'm surprised your app is working without modifying this. We might be able to add fs.strict: false here to just allow all. Any thoughts?

Would also be good to get a test demonstrating this new behavior which I can help you with.

@lmiller1990
Copy link
Contributor

lmiller1990 commented Feb 15, 2023

Copy pasting the reproduction @IlCallo supplied in Discord for some context

Notes about the repro:

- you'll need Yarn 1, unluckily we're still stuck with that
- clone https://github.com/IlCallo/quasar and checkout upgrade-cypress-ae
- cd ui
- yarn install
- cd dev
- yarn install
- cd ..
- yarn test:component
- notice Cypress can correctly detect tests
- click on any test, I did my tests on QAvatar ones
- notice the request returns 404, probably due to a wrong path 

Not sure if this fixes all issues in #22505, but it looks like it at least fixes Quasar, who ships us as part of their first class tooling, so I'd definitely like to see this one reviewed and merged (unless it is introduces a regression).

@lmiller1990 lmiller1990 removed the request for review from a team February 15, 2023 00:21
// Using relative path wouldn't allow to load tests outside Vite project root folder
// So we use the "@fs" bit to load the test file using its absolute path
const testFileAbsolutePathRoute = `${devServerPublicPathRoute}/@fs${CypressInstance.spec.absolute}`

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like @fs is a Vite thing - I couldn't find much documentation outside of https://vitejs.dev/config/server-options.html#server-fs-allow.

I'll update the snapshots - I don't know if this breaks anything, but I always thought the src/src duplication was kind of weird.

@lmiller1990
Copy link
Contributor

I took care of updating the snapshots ecdabcd

@ZachJW34 do you have any thoughts on the blast radius of this change? I suspect this is an implementation that shouldn't impact anyone (other than people experiencing a related bug, for whom it'll fix).

@IlCallo
Copy link
Contributor Author

IlCallo commented Feb 15, 2023

so I'm surprised your app is working without modifying this

I'm surprised too, but I'm not a Vite expert, so there's probably some underlying business rule I'm not aware of.
Could it be due to the fact that we provide our own vite dev server config?

Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

@lmiller1990 I think the blast radius is small. Stack traces are working properly and it fixes the bug (checked out the quasar repro to verify). It was cool to see that the specs list works as expected for file outside the projectRoot (with the ../)!

I'm good with this change!

@lmiller1990 lmiller1990 self-requested a review February 16, 2023 01:53
@lmiller1990
Copy link
Contributor

lmiller1990 commented Feb 16, 2023

Thanks @IlCallo, this will go in the next release, that'll be in ~2 weeks. I'll merge it up by the end of this week (need to do a few housekeeping things, like a changelog entry, etc).

@lmiller1990 lmiller1990 merged commit d54fa65 into cypress-io:develop Feb 17, 2023
mjhenkes added a commit that referenced this pull request Feb 21, 2023
* fix: update newProject ref when switching between organizations in SelectCloudProjectModal (#25730)

* chore: debug page tooltip distance and artifact border (#25727)

* misc: debug page tooltip distance and artifact border

* add changelog entry

* fix CT test

* fix: Improve error handling around calls to `this.next` in middleware (#25702)

* chore: update changelog validation example (#25742)

* misc: improve debug loading text wrap responsiveness (#25703)

* misc: Increase max failures in IATR badge to 99 (#25737)

* chore: exclude collaborator issues/PRs from triage project (#25769)

* feat: add --auto-cancel-after-failures flag (#25237)

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Matt Schile <mschile@cypress.io>
Co-authored-by: Ryan Pei <ryanppei@gmail.com>
Co-authored-by: Emily Rohrbough <emilyrohrbough@yahoo.com>

* chore: Update v8 snapshot cache (#25592)

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* Update update_v8_snapshot_cache.yml

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

* chore: updating v8 snapshot cache

---------

Co-authored-by: cypress-bot[bot] <2f0651858c6e38e0+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>
Co-authored-by: cypress-bot[bot] <47117332+cypress-bot[bot]@users.noreply.github.com>

* fix: implement new graphql fields for spec counts (#25757)

Co-authored-by: Stokes Player <stokes@cypress.io>
Co-authored-by: Mike Plummer <mike-plummer@users.noreply.github.com>

* feat: Bundle cy.origin() dependencies at runtime (#25626)

Co-authored-by: cypress-bot[bot] <2f0651858c6e38e0+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>

* chore: remove zenhub from release process (#25701)

Co-authored-by: Matt Schile <mschile@cypress.io>

* feat: add Cypress.Commands.overwriteQuery (#25674)

* feat: add Cypress.Commands.overwriteQuery

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <git@chary.us>

* fix: spawn child process with process.env in macOS arm64 (#25753)

Co-authored-by: Matt Schile <mschile@cypress.io>
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <github@chary.us>

* chore: lint system tests in CI (#25673)

* fix: Suppress filesystem errors during glob search (#25774)

* chore: issue with ts-loader missing in binary and problematic esbuild norewrite construct (#25797)

* chore: update changelog linting (#25809)

* docs(guides): add more detail to code-signing (#25794)

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

* chore: update workflows.yml to include the v8 snapshot update branch (#25784)

Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>

* chore: internal request preflight (#25772)

---------

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: cypress-bot[bot] <2f0651858c6e38e0+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>
Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
Co-authored-by: Zach Bloomquist <git@chary.us>

* chore: bump for 12.6.0 release (#25812)

* chore: release @cypress/webpack-batteries-included-preprocessor-v2.4.0

[skip ci]

* chore: release @cypress/webpack-preprocessor-v5.17.0

[skip ci]

* test: skip flaky GitDataSource test (#25825)

* chore: making our add-to-triage-board workflow reusable within the Cypress-io org (#25820)

* chore: Making our add to triage workflow callable from other projects inside the Cypress-io org in Github

* chore: updated cypress-example-kitchensink version (#25828)

* fix: duplicate and expired cookies (#25761)

* chore: add regression tests for duplicate cookies and bad expiry times

* avoid prepending domain with dot for cookies that are set with the server side jar. This is to avoid the cookie being duplicated if it is set or overridden in a different context (request that can actually set the cookie or via document.domain)

* feat: use cookie.toString() in the cookie patch to more accurately set cookies on the document, which should include other properties besides key=value

* fix: add logic to handle expired cookies in the document.cookie patch, as well as in CDP

* chore: build binary for cookie fixes for users to test

* chore: change name of fixture to something more accurate

* chore: comment why we are using the toughCookie toString method in the patch

* [run ci]

* chore: add changelog entry

* [run ci]

* fix: revert back to key=value when getting document.cookie as those are the only values are displayed (oversight on my end)

* [run ci]

* chore: make compatible with cypress.require

* fix: add tests for hostOnly/non hostOnly cookies to make sure property gets sent up to automation client correctly. No longer need custom cookie prop to determine destination

* [run ci]

* fix: stale unit test

* chore: adjust comments

* [run ci]

* fix: bad domain logic

* [run ci]

* chore: remove irrelevant comment

* [run ci]

* fix: adjust cookie login text to spec hostOnly cookie within the cookie patch. This should yield the same behavior as we are bound to same origin within the spec bridge

* [run ci]

* [run ci]

* fix: allow for cookies on request of same key to take precedence over cookies in the jar, regardless of how many hierachy cookies exist in the jar

* chore: fix cookie misc tests for cy.origin (dont run cy.origin)

* [run ci]

* chore: skip misc cookie tests in webkit as headless behavior doesn't clear cookies between tests correctly

* Revert "fix: allow for cookies on request of same key to take precedence over cookies in the jar, regardless of how many hierachy cookies exist in the jar"

This reverts commit 17de188.

* [run ci]

* chore: split changelog entry into two parts

* chore: update logic to remove else statement and add comments

* [run ci]

* chore: readd windows snapshot branch in workflows

* [run ci]

* chore: fix workflows from bad merge

* [run ci]

* Revert "chore: split changelog entry into two parts"

This reverts commit 4352ef5.

* [run ci]

* fix: Fix type definitions for cy.reload() (#25779)

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

* misc: Debug header updates (#25823)

* fix: allow running tests outside Vite project root folder (#25801)

* fix: allow running tests outside Vite project root folder

* update snapshots

* add changelog entry

---------

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

* fix: mount component in [data-cy-root] (#25807)

* fix(angular): mount component in [data-cy-root]

* fix e2e test

* add changelog entry

* changelog [skip ci]

* changelog

---------

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

* chore: updating add to triage baord github action to use org secret (#25868)

* chore: updating add to triage board github action to use org secret

* chore: release @cypress/angular-v2.0.2

[skip ci]

* chore: release @cypress/vite-dev-server-v5.0.3

[skip ci]

* chore: Update v8 snapshot cache (#25822)

Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>

* feat: support host only cookies (#25853)

* feat: allow setCookie API to take a hostOnly option

* chore: add jsdoc/typescript description to render to users

* chore: add changelog entry

* [run ci]

* chore: fix types

* chore: fix cookie login tests

* chore: update e2e cookie system tests

* [run ci]

* chore: fix cookie command tests. localhost cookies are calculated as hostOnly, which is consistent with how cypress works today

* chore: fix system tests for cookies.

* [run ci]

* chore: fix system tests

* chore: skip hostOnly assertions in webkit (for now)

* [run ci]

* chore: add property definitions to setCookieOptions

* [run ci]

* chore: add comments to hostOnly prop in firefox when setting a cookie

* fix(webpack-dev-server): touch component-index during onSpecsChange to avoid writing to app file (#25861)

* testing: try disabling uTimesSync and see what happens

* build binaries [run ci]

* fix: touch component index file instead of browser.js

* build binaries [run ci]

* update test

* fix test

* add test for custom HTML file in config

* use existing component index in webpack-dev-server unit tests

---------

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

* chore: release @cypress/webpack-dev-server-v3.2.4

[skip ci]

* chore: improve types for server automation cookie client (#25836)

* chore: improve types for automation cookies

* [run ci]

* fix: the cookie_behavior tests by syncing cookies immediately if … (#25855)

* fix: fix the cookie_behavior tests by syncing cookies immediately if the application is already stable

* chore: add changelog entry

* [run ci]

* chore: address comments from code review

* feat: Public API for CT Framework Definitions (#25780)

* chore: rework component onboarding in launchpad (#25713)

* chore: refactoring and types

* rework source of frameworks

* revert rename

* fix tests

* fix more tests

* types

* update code

* use same public API internally

* rename interfaces

* rename

* work on dev server api

* fix types

* fix test

* attempt to support getDevServerConfig

* tests

* add function to define framework [skip ci]

* rework a lot of types

* fix test

* update tests and types

* refactor

* revert changes

* lint

* fix test

* revert

* remove

* add "community" label [skip ci]

* refactor

* types

* lint

* fix bug

* update function name

* address feedback

* improve types with Pick

* refactor using type guard

* correct label

---------

Co-authored-by: Zachary Williams <ZachJW34@gmail.com>

* chore: typing error

* feat: scan for 3rd party ct plugins (#25749)

* chore: refactoring and types

* rework source of frameworks

* revert rename

* fix tests

* fix more tests

* types

* update code

* use same public API internally

* rename interfaces

* rename

* work on dev server api

* fix types

* fix test

* attempt to support getDevServerConfig

* tests

* add function to define framework [skip ci]

* rework a lot of types

* fix test

* update tests and types

* refactor

* revert changes

* lint

* fix test

* revert

* remove

* add "community" label [skip ci]

* refactor

* types

* lint

* fix bug

* update function name

* address feedback

* feat: scan for 3rd party ct plugins

* add e2e test

* unit tests [run ci]

* tweak resolution

* rebase, address comments

* fix windows paths

* remove .gitignore

* fix test

---------

Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>

* lint config

* spacing

* try fix race cond

* fix import error

* build binary

* try update snapshot

* try using require

* support namespaced definitions (#25804)

* remove category

* add icon prop

* support esm -> cjs compiled typescript

* fix test

* misc: add CTA footer to launchpad framework dropdown (#25831)

* remove test project dependencies

* rebase

* windows

* windows again

* add changelog entry

* changelog

* revert workflow

* remove worklfow

---------

Co-authored-by: Zachary Williams <ZachJW34@gmail.com>
Co-authored-by: Adam Stone-Lord <adams@cypress.io>

* chore: release @cypress/webpack-dev-server-v3.3.0

[skip ci]

* fix: Add missing error message when `req.continue` is used incorrectly (#25884)

---------

Co-authored-by: Adam Stone-Lord <adams@cypress.io>
Co-authored-by: Zachary Williams <ZachJW34@gmail.com>
Co-authored-by: Mike Plummer <mike-plummer@users.noreply.github.com>
Co-authored-by: Matt Schile <mschile@cypress.io>
Co-authored-by: Alejandro Estrada <estrada9166@gmail.com>
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Ryan Pei <ryanppei@gmail.com>
Co-authored-by: Emily Rohrbough <emilyrohrbough@yahoo.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: cypress-bot[bot] <2f0651858c6e38e0+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Ryan Manuel <ryanm@cypress.io>
Co-authored-by: cypress-bot[bot] <47117332+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Mark Noonan <mark@cypress.io>
Co-authored-by: Stokes Player <stokes@cypress.io>
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <git@chary.us>
Co-authored-by: willmsC <50909991+willmsC@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <github@chary.us>
Co-authored-by: cypress-bot[bot] <+cypress-bot[bot]@users.noreply.github.com>
Co-authored-by: Tim Griesser <tgriesser10@gmail.com>
Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
Co-authored-by: Ben M <benm@cypress.io>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
Co-authored-by: Podles <78863563+podlesny-j@users.noreply.github.com>
Co-authored-by: Paolo Caleffi <p.caleffi@dreamonkey.com>
Co-authored-by: Lachlan Miller <lachlan.miller.1990@outlook.com>
@IlCallo IlCallo deleted the allow-tests-outside-vite-root-folder branch March 6, 2023 15:12
@IlCallo
Copy link
Contributor Author

IlCallo commented Mar 6, 2023

@ZachJW34 @lmiller1990
I dug a bit further and found out why it works for Quasar repo tests: we tamper with the viteConfig of our project adding .. into server.fs.allow

https://github.com/quasarframework/quasar/blob/8f189d3f6b54d083cb40da8bf809d38dd120faa5/ui/dev/quasar.config.js#L80-L82

The change in this PR is still relevant, but it may be a good idea to specify somewhere on the docs that customizing the server.fs.allow property is needed to access files outside what Cypress consider the project root folder

Unless there are security concerns, removing the server.fs.allow altogether could be an option too.
I guess it depends on why it has been added in the first place

@lmiller1990
Copy link
Contributor

I suspect we should not tamper with server.fs.allow, since it's related to opt in file system access (I think we a third party library should not mess with this) but we can definitely add a note to the docs around this.

@IlCallo
Copy link
Contributor Author

IlCallo commented Mar 7, 2023

suspect we should not tamper with server.fs.allow

Playing the guess game, I'd say you added that option because since Vite 2.7 that option is enabled by default and only allows to access files in the current workspace(s), but you have some files you need which are outside the project root folder (as we have), especially the Cypress binary stored in the global cache

Another related problem which recently came up is that you aren't re-adding the workspaces paths to the allow after overriding the option, which cause problems when using Cypress component testing to execute all tests into a monorepo with multiple packages: #26036

@mike-plummer
Copy link
Contributor

@lmiller1990 Potentially related issue & PR opened to work around the server.fs.allow prop in monorepos, in case you're curious

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.

None yet

6 participants