-
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
feat: make cypress.config.js available #17000
Conversation
Thanks for taking the time to open a PR!
|
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 |
This might make migration to the new cypress.js (with plugins support) more difficult, causing users to do two config migrations back-to-back. Especially if we end up going with cypress.config.js. thoughts @cowboy @mccataldo ? |
@bkucera I agree 💯
I am sorry for the initial confusion |
I share the concern that Ben mentioned. It might be more confusing to introduce one way of writing cypress.config.js and then another way than just to introduce the final way. Also, a reason for utilizing the plugins file architecture for cypress.config.js is that it's contained in a child process and has more robust error handling. If there's an asynchronous error in this implementation of cypress.config.js, Cypress will crash. With the plugins file architecture, it would display an appropriately tailored error message.
What is the plan for releasing this? |
I think it would be wise to coordinate any proposed changes to |
I'm also not understanding the purpose behind this change if it is not meant to be user facing before we make the changes to include plugin functionality to cypress.js. I read all of the Jira stories and the entire Notion doc and didn't see this as a step where we were headed. I may have missed it though. |
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.
Left some questions, but looks pretty solid!
npm/vue/cypress.config.js
Outdated
@@ -0,0 +1,32 @@ | |||
const { startDevServer } = require('@cypress/webpack-dev-server') |
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.
Cool, in future we should try to use git mv
to preserve the history. It will also make the PR smaller to review.
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.
In theory yes, but have you seen it actually work?
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.
Usually, but I had an experience recently where it did not - seems if you change too many LOC in the file, it doesn't work. Not really a big deal, but good to keep in mind.
npm/vue/cypress.config.js
Outdated
projectId: '134ej7', | ||
testFiles: '**/*spec.js', | ||
experimentalFetchPolyfill: true, | ||
component: (on, config) => { |
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.
If we want to customize some values for component, eg viewport, what is the recommendation?
Before
{
"component": {
"viewportHeight": 400
}
}
After
module.exports = {
component: (on, config) => {
// ...
return {
...config,
viewportHeight: 400
}
}
}
Is this correct? For anyone with overrides, the migration won't be as simple as rename + wrap in module.exports
. We might want to include this in the PR description and (eventual) docs.
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 think this is how it was intended indeed.
This or a simple affectation (no need for the spread)
module.exports = {
component: (on, config) => {
// ...
config.viewportHeight = 400
return config
}
}
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. Just mentioning a few edge cases I found. I didn't really test too extensively. I'm not sure if all of these issues are something that need addressing now or if you want to make separate tickets for future work.
Crash
I had Cypress crash at one point when I was editing the cypress.config.js
file and now I can't reproduce it. 😞 There was no error in the stdout when it happened.
Export empty obj
If you export an empty object for the cypress.config.js
file with a cypress.json
file present - it sees this as if the cypress.config.js
has been deleted completely and a misleading error displays. This should be supported and is likely the large majority of cases of how I see people would start the file when migrating.
module.exports = {}
nonexistent configFile
If I pass a nonexistent --config-file
via cypress open
, there's a pretty confusing error that displays in the GUI. The previous behavior was that it created a new file at the location. I actually don't remember that being the behavior or if that's what we want to continue doing.
Additionally when I click to 'Try again' on this screen (still with the nonexistent file), Cypress gets stuck in a loading spinner and clicking into the other tabs just blank out - have to restart Cypress.
Settings
The Settings tab is pretty wonky.
e2e shows as null even though I have a function set
These extra values are showing
These values don't show in 7.6.0.
configFile should show as relative path
The instructions for where to put projectId need updating
Also Node.js instructions need updating
projectId
I'm curious on what would happen if someone exportts a different projectId for e2e versus component - will need to be considered and tested as I imagine someone would try to set up 2 different projects in the Dashboard.
@@ -830,7 +830,7 @@ describe('src/cy/commands/request', () => { | |||
expect(this.logs.length).to.eq(1) | |||
expect(lastLog.get('error')).to.eq(err) | |||
expect(lastLog.get('state')).to.eq('failed') | |||
expect(err.message).to.eq('`cy.request()` must be provided a fully qualified `url` - one that begins with `http`. By default `cy.request()` will use either the current window\'s origin or the `baseUrl` in `cypress.json`. Neither of those values were present.') | |||
expect(err.message).to.eq('`cy.request()` must be provided a fully qualified `url` - one that begins with `http`. By default `cy.request()` will use either the current window\'s origin or the `baseUrl` in `cypress.config.js`. Neither of those values were present.') |
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.
This error is slightly inaccurate since the baseUrl could have been passed via the --config outside of the configuration file. But, I know this wasn't introduced in this 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.
I fixed the error message to say option instead of the file, since it can com from either configfile or the CLI options but is most likely something wrong within the test.
@jennifer-shehane this one is on the main branch too I believe. It is linked to the configuration of typescript. I fixed it in my last commit. |
Great review above - can you tag me again when I should re-review this? Looks like you are still addressing some feedback and CI is red. |
@jennifer-shehane thank you for the great review. I fixed the error when a config file was missing or misnamed. I am very hesitant to fix the
[EDIT] Since we decided to merge this in 8.X, I did all the nececssary updates in the desktop-gui to avoid inconsistencies. The 3 points above are therefore irrelevant. |
This code is supposed to watch the config file and reload if it or any required files change, I believe? This seems like a bug: cypress/packages/server/lib/project-base.ts Lines 559 to 561 in 69ebdba
|
@flotwig the code you linked looks like it should be working. It's checking if |
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.
Transfering the npm directories to the new cypress.config.js should be done in a separate PR considering this is not a breaking change. It's best to keep the PR to be the bare minimum required for the feature. The PR is already very large and it would help get a faster review since this requires every member of the team to review. Also any refactors like the markdown-renderer refactor should be in a separate PR.
I've left comments within the code for review. Please expand all hidden comments, there is a lot.
Outside of those, some observations while manually testing:
-
When the error is shown about duplicate config, the loading spinner for the projects area is still shown and spinning.
-
Can we reword this message instead of saying 'retry again' to explicitly tell them what to do.
Please remove one of the two, quit and relaunch Cypress.
-
So I just pasted the JSON from the
cypress.json
file directly into thecypress.config.js
file, because I kind of expect people to just do this without reading any of the migration guide and I get this error. I'm not sure if the error here can be better. Is there a way to detect that they just have JSON in the file and explain to them that if it's pure JSON that they need to migrate to export, etc - link to migration guide? -
When I added the export as shown in the PR description, I got this error about unexpected token, 'export'.
-
Then I tried to switch to the
defineConfig
method I'd seen in the config.ts in this repo and it throws an error again. I guess my bit of takeaway from this is that these error messages are not helpful at all. They should say something like:There's an error in your config file at the following location: ...
Make sure that your config file is structured like this:
...minimum required code structure for file....
link to guide where it explains how to write in config file.
And secondly apparently I have no idea how to write this config file because this PR description is not up to date. Please keep the PR description up to date so people can find the proper documentation to fully review without having to search through outside resources.
<img width="515" alt="Screen Shot 2021-08-24 at 10 29 33 AM" src="https://user-images.githubusercontent.com/1271364/130645482-10513f7e-f090-4778-bfad-cc303d24bda8.png">
<img width="798" alt="Screen Shot 2021-08-24 at 10 30 17 AM" src="https://user-images.githubusercontent.com/1271364/130645572-b17a8f2a-3a60-4c82-b0a6-c35815404194.png">
- Was just thinking through another case where the cypress.json and
cypress.config.js
will co-exist together. If someone runs Cypress on 8.x (wherecypress.config.js
is scaffolded), then quickly runs Cypress on 8.3.0 to test some change in the update, it will scaffold thecypress.json
back again. May be annoying for a few people trying to track down some regression for it to scaffold a diff file for each back and forth.
packages/server/lib/errors.js
Outdated
// TODO: update with vetted cypress language | ||
case 'CT_NO_DEV_START_FUNCTION': | ||
return stripIndent`\ | ||
To run component-testing, cypress needs the \`setupDevServer\` function to be implemented. |
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.
To run component-testing, cypress needs the \`setupDevServer\` function to be implemented. | |
To run component-testing, the \`setupDevServer\` function needs to be implemented. |
packages/server/lib/errors.js
Outdated
return stripIndent`\ | ||
To run component-testing, cypress needs the \`setupDevServer\` function to be implemented. | ||
|
||
Add a \`setupDevServer()\` in the component object of the ${arg1} 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.
Add a \`setupDevServer()\` in the component object of the ${arg1} file. | |
Add a \`setupDevServer()\` in the `component` object of the ${arg1} file. |
@@ -2582,6 +2594,7 @@ declare namespace Cypress { | |||
/** | |||
* If set to `system`, Cypress will try to find a `node` executable on your path to use when executing your plugins. Otherwise, Cypress will use the Node version bundled with Cypress. | |||
* @default "bundled" | |||
* @deprecated nodeVersion will soon be fixed to "system" to avoid confusion |
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 don't think this sufficiently explains the reasoning for the move and would rather avoid putting a short description that is not all encompassing of the reasons.
* @deprecated nodeVersion will soon be fixed to "system" to avoid confusion | |
* @deprecated nodeVersion will soon be fixed to "system" |
if (typeof cfg.configFile === 'string' | ||
&& /\.json$/.test(cfg.configFile) |
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.
if (typeof cfg.configFile === 'string' | |
&& /\.json$/.test(cfg.configFile) | |
const = isJsonFile = (filename) => { | |
return /\.json$/.test(cfg.configFile) | |
} | |
if (typeof cfg.configFile === 'string' | |
&& isJsonFile(cfg.configFile) |
process.chdir(this.projectRoot) | ||
|
||
// TODO: we currently always scaffold the plugins file | ||
// even when headlessly or else it will cause an error when | ||
// we try to load it and it's not there. We must do this here | ||
// else initialing the plugins will instantly fail. | ||
if (cfg.pluginsFile) { | ||
if (cfg.pluginsFile && (!cfg.configFile || /\.json$/.test(cfg.configFile))) { |
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.
if (cfg.pluginsFile && (!cfg.configFile || /\.json$/.test(cfg.configFile))) { | |
if (cfg.pluginsFile && (!cfg.configFile || isJsonFile(cfg.configFile))) { |
packages/server/lib/util/settings.js
Outdated
// we cannot write due to folder permissions | ||
return errors.warning('FOLDER_NOT_WRITABLE', projectRoot) | ||
}).catch((err) => { | ||
if (errors.isCypressErr(err)) { | ||
debug('throwing error') |
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.
debug('throwing error') | |
debug('error thrown when seeing if find out if "%s" exists', file) |
packages/server/lib/util/settings.js
Outdated
}).catch({ code: 'EACCES' }, { code: 'EPERM' }, () => { | ||
return this._err('CONFIG_FILE_NOT_FOUND', this.configFile(projectRoot, options), projectRoot) | ||
}).catch({ code: 'EACCES' }, () => { | ||
debug('no access error') |
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.
debug('no access error') | |
debug('no access error for "%s"', file) |
packages/server/lib/util/settings.js
Outdated
@@ -196,7 +288,9 @@ module.exports = { | |||
return Promise.resolve({}) | |||
} | |||
|
|||
return this.read(projectRoot) | |||
debug('write a 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.
You need to think about these debug logs being logged alongside thousands of logs. It's helpful to be descriptive for support later to search/find the exact place where this debug log is. 'write a file' is too generic imo.
debug('write a file') | |
debug('write a new configuration file') |
Thanks for the review @jennifer-shehane. Agree on keeping this one as minimal as possible. Since Bart is away I'll raise this in our team and someone else will pick it up. |
Thank you @jennifer-shehane Two details I wanted whomever is taking over the pull request need to know:
|
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.
Jennifer and Lachlan did pretty thorough reviews, so I don't have too much to add besides some nitpicks.
@@ -0,0 +1,76 @@ | |||
require('graceful-fs').gracefulify(require('fs')) | |||
const debug = require('debug')('cypress:server:require_async:child') |
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.
Should be possible for this to be TS. If CYPRESS_INTERNAL_ENV === 'development'
, utilize @packages/ts/register.js
when spawning the child process (example here). Otherwise, for production, tsc
the TS files and ship JS files in the binary, same as we do for the test of the TS files.
@@ -55,6 +55,115 @@ describe('lib/plugins/util', () => { | |||
|
|||
expect(handler).not.to.be.called | |||
}) | |||
|
|||
context('#arguments-serialization', () => { |
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.
context('#arguments-serialization', () => { | |
context('argument serialization', () => { |
#
is meant to denote a method. It's not needed if it's a more generic suite title.
}) | ||
}) | ||
|
||
context('#arguments-deserialization', () => { |
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.
context('#arguments-deserialization', () => { | |
context('argument deserialization', () => { |
@@ -55,6 +55,115 @@ describe('lib/plugins/util', () => { | |||
|
|||
expect(handler).not.to.be.called | |||
}) | |||
|
|||
context('#arguments-serialization', () => { | |||
it('#send should send functions arguments as a serialized string', function () { |
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.
Seeing all these tests prefixed with #send
indicates they could be moved into a describe
called #send
, then the test title is specific to what's unique about that test.
Also, it's not necessary to write "should" in every test title. It becomes redundant.
}) | ||
|
||
context('#arguments-deserialization', () => { | ||
it('#on should deserialise function strings into function', function () { |
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.
Same as with #send
, #on
should be moved out into a describe
.
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
closes
https://cypress-io.atlassian.net/browse/UNIFY-18
https://cypress-io.atlassian.net/browse/UNIFY-208
https://cypress-io.atlassian.net/browse/UNIFY-209
https://cypress-io.atlassian.net/browse/UNIFY-247
https://cypress-io.atlassian.net/browse/UNIFY-248
https://cypress-io.atlassian.net/browse/UNIFY-249
https://cypress-io.atlassian.net/browse/UNIFY-250
https://cypress-io.atlassian.net/browse/UNIFY-17
https://cypress-io.atlassian.net/browse/UNIFY-264
https://cypress-io.atlassian.net/browse/UNIFY-212
closes #5218
Summary
In preparation for the unified runner project, we need to be able to detect which testing types have been setup.
For this detection, we plan on using
e2e
andcomponent
function. So we need those functions to be there before unification.cypress.jsoncypress.config.tsTo configure cypress, we can now use
cypress.config.ts
orcypress.config.js
instead ofcypress.json
.Using a
ts
orjs
file to store config needs cypress to execute/evaluate the file in a node context.Since this is user code, this could throw a parsing/runtime error and crash the electron runner.
To avoid crashing, we spawn another process to parse the file. It allows cypress to elegantly handle each error.
One downside of using a separate process is this:
Functions in this config file cannot be transmitted to the main process.
We don't get the new e2e and component functions in the config object passed around everywhere.
It matters very little since they are supposed to be run in another child process anyway (see run_plugin.js) but it is worth noting.
Default config file
Three default config files are now possible:
cypress.json
,cypress.config.js
andcypress.config.ts
. If more than one of those files is present at the root of the project, the system errors because of abiguity.Creating missing file
Before, when the targeted config file did not exist cypress would create it and populate it with the arguments passed to the CLI.
It still does that if a json config file is specified in the CLI. It creates a config file by default if a
js
orts
file is specified and not found. By default, it's going to create acypress.config.js
file.Plugins definition functions
Before, all the servers and preprocessors had to be defined in the
pluginsFile
. This js/ts file whose path was configured incypress.json
was executed when loading a project on the cypress server. It allowed users to modify the cypress config depending on execution context and setup preprocessors, tasks and event handlers. It exported a function like below.Though we can still do that, now we can set this up directly in the
cypress.config.ts
.Note how we setup the dev-server for component testing as a separate function
setupDevServer
.If a plugins function and a pluginsFile are implemented, the system will throw an error. We want to avoid halfway migration.
Technical implementation
When it gets time to execute the
pluginsFile
, ifconfig.component
is a function,we replace the configured
pluginsFile
withconfigFilePath
and tell the plugins resolver what function it is supposed to execute.Else, I let the pluginsFile run as before.
For now, if neither the
pluginsFile
nor the functions incypress.config.ts
exist, cypress creates sample plugins file. This avoids breaking tests but encourages the creation of a deprecated configuration.This should be avoided.
defineConfig()
Vite, Vue CLI 4, windicss and a few other libraries have implemented a pattern for configuring their APIs that is very practical: a
defineConfig()
function.This enables autocompletion and inline documentation in the config file while minimizing boilerplate.
Here is how to use it.
desktop-gui
As a consequence of these changes, the desktop-gui was displaying some wrong info in the settings tab.
Here are my fixes:
Deprecation warnings
To give a heads up about this necessary change. We add a warning both in the component testing runner and in the desktop GUI.
To see the desktop warning:
yarn start
To see the warning in the component testing runner
cd npm/react/examples/react-scripts
yarn cy:open
Technical detail
We had to wire up the component testing runner to receive warnings in the first place.
Look in
server-ct/src/index.ts
to see how it was done.Remaining work before merge
These tasks remain once this change is approved.
They have to be accomplished before we deliver this to users to give them a solution when they see the deprecation.