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

feat: add cloud recommendation message to CI output #24680

Merged
merged 24 commits into from Nov 18, 2022

Conversation

astone123
Copy link
Contributor

@astone123 astone123 commented Nov 14, 2022

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:

  • Cypress is in run mode
  • The process is running in CI (using is-ci library to determine this)
  • CYPRESS_COMMERCIAL_RECOMMENDATIONS environment variable is not set to 0
  • At least one test in the run failed
  • --record flag 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.io from 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.js and run those

How has the user experience changed?

Screen Shot 2022-11-17 at 2 46 33 PM

PR Tasks

@cypress-bot
Copy link

cypress-bot bot commented Nov 14, 2022

Thanks for taking the time to open a PR!

@astone123 astone123 changed the title Astone123/cloud recommendation message feat: add cloud recommendation message to CI output Nov 14, 2022
@cypress
Copy link

cypress bot commented Nov 14, 2022

@astone123 astone123 self-assigned this Nov 14, 2022
@astone123 astone123 requested review from a team and pstakoun Nov 14, 2022
@astone123 astone123 marked this pull request as ready for review Nov 14, 2022
Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

I think there is logic bug - please confirm I'm not going crazy.

Other than that, code and tests look 💯

packages/server/lib/util/print-run.ts Outdated Show resolved Hide resolved
packages/server/lib/util/print-run.ts Outdated Show resolved Hide resolved
packages/server/lib/util/print-run.ts Outdated Show resolved Hide resolved
packages/server/lib/util/print-run.ts Outdated Show resolved Hide resolved
@astone123 astone123 requested a review from a team Nov 15, 2022
Copy link
Contributor

@ZachJW34 ZachJW34 left a comment

Choose a reason for hiding this comment

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

Tested it out, works great! Couple small comments

packages/server/lib/util/print-run.ts Outdated Show resolved Hide resolved
packages/server/test/integration/cypress_spec.js Outdated Show resolved Hide resolved
@ZachJW34
Copy link
Contributor

@astone123 Does deleting the CircleCI CYPRESS_NO_COMMERCIAL_RECOMMENDATIONS have any impact on this being merged?

@astone123
Copy link
Contributor Author

@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

@astone123
Copy link
Contributor Author

Waiting on this for now - going to meet with @pstakoun and Brian tomorrow to get final approval

@lmiller1990 lmiller1990 self-requested a review Nov 16, 2022
Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

.

@brian-mann brian-mann self-requested a review Nov 16, 2022
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

  • Might want to switch to using the is-ci npm 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_REPORTS since 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

@astone123 astone123 requested a review from brian-mann Nov 17, 2022
@@ -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',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do these need to be in the force-no-rewrite category?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@astone123 astone123 merged commit bf6a52a into develop Nov 18, 2022
82 of 83 checks passed
@astone123 astone123 deleted the astone123/cloud-recommendation-message branch Nov 18, 2022
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.

Add cloud recommendation message to CI output
5 participants