feat: add cloud recommendation message to CI output #24680
Conversation
|
Thanks for taking the time to open a PR!
|
astone123
changed the title
|
@astone123 Does deleting the CircleCI |
|
@ZachJW34 yeah, I need that variable in the project in order for the tests to pass, so I need to put it back before I push a follow-up commit, but that will cause everyone else's tests to fail until this branch gets merged... I'll put something in Slack to explain the situation once this is ready to merge |
|
Waiting on this for now - going to meet with @pstakoun and Brian tomorrow to get final approval |
…3/cloud-recommendation-message
There was a problem hiding this comment.
- Might want to switch to using the
is-cinpm utility since we use that elsewhere - Consider where to document this env var in the docs (maybe installation, with another section in Configuration that enumerates env vars that control behavior - with links to other areas)
- The name of the env var sounds good - we may want to align this to the existing
CYPRESS_CRASH_REPORTSsince that doesn't use the "NO" verbiage - Instead of hard coding the line dividers, go through the same utility methods bc I believe they dynamically adjust their width to the available space in the terminal
- Let's iterate on the colors, formatting, line dividers, to make the message less aggressive
- After iterating just drop in a picture of the formatting and we can do async reviews
| @@ -69,4 +69,7 @@ export default [ | |||
| 'node_modules/prettier/parser-meriyah.js', | |||
| 'node_modules/prettier/parser-typescript.js', | |||
| 'node_modules/prettier/third-party.js', | |||
| 'packages/server/node_modules/is-ci/index.js', | |||
There was a problem hiding this comment.
Why do these need to be in the force-no-rewrite category?
There was a problem hiding this comment.
I'm trying to figure out exactly why, but it wouldn't work without adding those (they probably don't all need to be in there).
I think what might be happening is that the exported value from is-ci is getting evaluated and snapshotted before process.env.CI=true is set, and then it never gets re-evaluated?
If I don't have these in this file then is-ci returns false even though we are running in CI and it should be true. @ZachJW34 originally suggested trying this when we were working through it
There was a problem hiding this comment.
@astone123 and I paired on this a bit. Not sure we need every one of these but it wasn't working without this addition. I believe it's due to how is-ci and ci-info are written, where the export is evaluated upon require e.g. module.exports = !!process.env.CI and not a function.
Could be totally wrong though!
There was a problem hiding this comment.
I think from playing around locally all we need to add here is the first two and I'm good with that for now. Though I still want to figure a little more on why.
User facing changelog
n/a
Additional details
This PR adds a message that prompts the user to go to Cypress Cloud after the test summary table only when all of the following conditions are true:
is-cilibrary to determine this)CYPRESS_COMMERCIAL_RECOMMENDATIONSenvironment variable is not set to0--recordflag was not passed to the cypress run command (the user is not attempting to record this run to Cypress Cloud)Services PR for on link: https://github.com/cypress-io/cypress-services/pull/4972
Docs PR: cypress-io/cypress-documentation#4863
Steps to test
Look at the output from this run and verify that the cloud recommendation message appears after the test summary. Note that Circle CI is redacting
cypress.iofrom the on link. This is a setting in our Circle CI project and shouldn't be an issue in other's projects.You can also look at the tests that I added in
cypress_spec.jsand run thoseHow has the user experience changed?
PR Tasks
cypress-documentation? feat: Add docs section about opting out of commercial messaging cypress-documentation#4863type definitions?