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: Suppress filesystem errors during glob search #25774

Merged
merged 4 commits into from
Feb 13, 2023

Conversation

mike-plummer
Copy link
Contributor

Additional details

Currently, our glob searches for project specs and components for "Create from Component" will fail if a filesystem error is encountered (such as a permission denied) and return an empty array. This can happen even if there's a deeply nested resource that has nothing to do with Cypress that is permissions-restricted or has a bad symlink. This PR updates the behavior to log and retry the glob search if an error is encountered so the user can at least get partial results even if there is a part of their specPattern that cannot be traversed.

Unfortunately there isn't an elegant way to "watch and log" in Globby. It only exposes a "suppress all errors" flag or we can roll our own filesystem adapter which felt heavy for this use case (and wrapping fs functions is tricky due to extensive use of callback args for error-handling). The best approach I could find was to try it once and, if an error is encountered, log and try again with errors suppressed. If the retry fails then we bail out with our current behavior of "log and return empty array".

Steps to test

  1. Create a new Cypress CT project using React or Vue. Make sure it has at least one component and at least one spec file
  2. Create a new directory within the project and remove read permission from it:
    mkdir secretDir
    chmod -r secretDir
  3. Launch latest Cypress version in CT open mode (npx cypress open --component), observe no specs are listed
  4. Select "Create from Component", observe no components are listed
  5. Close Cypress and re-open project using this branch with DEBUG=cypress:data-context:sources:FileDataSource
  6. Observe that spec file is listed, retry "Create from Component" and observe component(s) are found
  7. Observe that filesystem EACCES error was logged with comment regarding retry

Note: This issue also impacts e2e, instructions can be adapted to validate e2e is also impacted/fixed by creating restricted directory under cypress/e2e to match default e2e spec pattern

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@mike-plummer mike-plummer requested a review from a team February 10, 2023 20:45
@cypress
Copy link

cypress bot commented Feb 10, 2023

14 flaky tests on run #44050 ↗︎

0 9401 953 0 Flakiness 14

Details:

Merge branch 'develop' into mikep/24109-ignore-filesystem-errs
Project: cypress Commit: 79e678b3d2
Status: Passed Duration: 15:52 💡
Started: Feb 13, 2023 10:09 PM Ended: Feb 13, 2023 10:25 PM
Flakiness  e2e/origin/cookie_behavior.cy.ts • 4 flaky tests • 5x-driver-electron

View Output Video

Test
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry
Flakiness  cypress/proxy-logging.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test
... > flags > req + res modified flagged as expected
Flakiness  commands/net_stubbing.cy.ts • 3 flaky tests • 5x-driver-webkit

View Output Video

Test
network stubbing > intercepting request > can delay and throttle a StaticResponse
... > with `times` > only uses each handler N times
... > stops waiting when an xhr request is canceled
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-webkit

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry

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

// results even if the glob search hits permission issues (#24109)
debug('Error in getFilesByGlob %o, retrying with filesystem errors suppressed', e)

return await this.getFilesByGlob(cwd, glob, { ...globOptions, suppressErrors: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just do this by default instead of the "if it fails, retry and suppress errors"? Any downside to that approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my original plan, but if we do that we get zero logging of the problem. My fear is that a user will have a spec file within a restricted directory and that spec will just never show up and there will be no logging (debug or otherwise) explaining why which will make it very difficult to troubleshoot.

Taking the "retry with suppress" approach is less performant, but gives us the possibility of debug logs.

@mike-plummer mike-plummer requested a review from a team February 13, 2023 14:32
Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

Tested as described in the issue and this works great.

@marktnoonan
Copy link
Contributor

@mike-plummer is it expected to still have problems running a spec that's now reachable? The spec is not inside the secretDir but its existence still appears to be a problem when parsing the support file...

At least this tells the user what the exact problem is so they can fix it.

Screenshot 2023-02-13 at 4 16 34 PM

@mike-plummer
Copy link
Contributor Author

@mike-plummer is it expected to still have problems running a spec that's now reachable? The spec is not inside the secretDir but its existence still appears to be a problem when parsing the support file...

At least this tells the user what the exact problem is so they can fix it.

@marktnoonan Interesting, I must've only tested this on Webpack because I hadn't seen that issue before. This is intended only to fix the "no specs at all show up" issue in the Specs List. As you point out, the error is appropriately surfaced to the user in this scenario so I'm not concerned

@mike-plummer mike-plummer merged commit 2c35510 into develop Feb 13, 2023
@mike-plummer mike-plummer deleted the mikep/24109-ignore-filesystem-errs branch February 13, 2023 22:21
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>
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.

Cypress does not find any component testing specs when folder matching glob can not be accessed
4 participants