-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use configured downloadsFolder
for Cypress
#164
Conversation
Co-authored-by: Andrew Ortwein <[email protected]>
Co-authored-by: Andrew Ortwein <[email protected]>
4312a9e
to
6144340
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences🚀 Don’t miss a bit, follow what’s new on Codacy. Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
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.
⬇️🗂️ Changes look good to me 👍🏻
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.
Nice work @tevanoff! Glad that awkward ./cypress/downloads/some
business I had written in is now gone.
// outputDir gives us the test-specific subfolder (https://playwright.dev/docs/api/class-testconfig#test-config-output-dir); | ||
// we want to write one level above that | ||
const finalOutputDir = join(outputDir, '..', 'chromatic-archives'); | ||
const finalOutputDir = join(outputDir, 'chromatic-archives'); |
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.
@tevanoff it seems like the directory traversal wouldn't be as much of a risk since people are configuring their own outputDir
, which looks on "their" OS/container for files. Is it worth making this change to be totally safe, or is outputDir
a relative path?
@skitterm, Todd brought up that we don't currently have any unit tests that verify things are written to a configured |
@andrewortwein @tevanoff my gut says that it's not worth the time to add the tests, where we didn't see an easy way of putting them in. |
packages/cypress/tests/cypress/e2e/custom-downloads-directory.cy.ts
Outdated
Show resolved
Hide resolved
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.
A couple small questions, but I'm glad we're able to cover this with tests without too much hassle!
packages/cypress/tests/cypress/e2e/custom-downloads-directory.cy.ts
Outdated
Show resolved
Hide resolved
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.
Looks good!
Issue: #
What Changed
The Cypress integration was hardcoded to use Cypress's default value for
downloadsFolder
,cypress/downloads
. Changes to that in the Cypress config would not be respected, and archive files would always be written to the default.This fixes that by reading the correct value from the Cypress config object.
How to test
downloadsFolder
to a non-default value incypress.config.ts
cypress/downloads