-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Support experimental features #6265
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
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
This comment has been minimized.
This comment has been minimized.
UI reviewDesktop GUI experiments tab "Learn more" link goes to (non-existent yet) https://on.cypress.io/experiments Experiment enabledNo experiments enabled
|
UI ReviewHeadless run with experiments enabled (without experiments same terminal output)
|
I'm gonna work on the Desktop-GUI to freshen the design up for the experiements flags display. |
Feedback - changes requested
|
@bahmutov I would camel case the experiments...
|
When we talked about this earlier, I assumed this would be a nested object, which seems cleaner to me than a bunch of very long config values starting with
Looked at some other tools cause I wasn't totally satisfied.
I think what looks weird is the camelCasing. Nobody camelCases the values but I get wanting to stick with our camelCase convention. |
Clicking into the Experiments panel completely borks if you do not have any experiments defined. I've updated the title to be |
I started to write test in the Desktop Gui specs for the Experiments panel, but not nearly enough. I also updated the code to iterate through 'experiments' and not blow up when there are no experiment variables defined. This code is a bit naive. We won't be able to pull any key starting with We also need to build up more properties for each experiment for the display:
|
@jennifer-shehane did you push any code? I don't see any commits. I love the UI with toggle - but these are read-only, right? since the user has to change them using We still control the experiments, thus the user cannot just put any As far as props for experiments: 'title/name', 'value', 'default value', 'description' - we have
|
… experiments-6257
@jennifer-shehane I have reworked the PR to be more to your liking - all experimental summaries and names are in a single place in server/lib/experiments.ts file and experiments.jsx is all dynamic and nothing is hardcoded. |
|
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.
Overall this PR looks good to me, barring approval for the design changes on the 'Experiments' panel
User facing changelog
Additional details
process.env.CYPRESS_EXPERIMENTS
. Unknown experiments are ignoredpackages/server/lib/experiments.ts
EXPERIMENTS
from resolved config and parse it using the same logic frompackages/server
. Which means technically the experiment can be overwritten using plugins file or CLI for the browser and desktop GUI, while only getting value from the environment variable on the Node sideHow has the user experience changed?
There is a new tab and by default, since there is nothing whitelisted in the config, it is just notification text
PR Tasks
Then:
cypress-documentation
?Plan is to merge this PR with
componentTesting
or empty list of experiments (?) and then merge #5923 withcomponentTesting
experiment (and a few tests for new mounting)