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 CYPRESS_SKIP_VERIFY option to suppress verification #29836

Merged
merged 15 commits into from
Aug 16, 2024

Conversation

joshuajtward
Copy link
Contributor

@joshuajtward joshuajtward commented Jul 11, 2024

Additional details

The PR adds a flag CYPRESS_SKIP_VERIFY to prevent the verification steps from running. These steps are semi-redundant in CI environments, and regularly timeout leading to false negatives in test suites.

Steps to test

  • Delete the Cypress cache on your machine and run Cypress with CYPRESS_SKIP_VERIFY=true in your env
  • Confirm that the verification messages are not shown ("Looks like this is your first time running Cypress N.N.N...")

How has the user experience changed?

If a user opts in they will not get the verification on Cypress startup

PR Tasks

@CLAassistant
Copy link

CLAassistant commented Jul 11, 2024

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

@jennifer-shehane jennifer-shehane changed the title misc: add CYPRESS_SKIP_VERIFY option to suppress verification feat: add CYPRESS_SKIP_VERIFY option to suppress verification Jul 24, 2024
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

changelog does not match code

cli/CHANGELOG.md Outdated Show resolved Hide resolved
@MikeMcC399
Copy link
Contributor

@joshuajtward

Additional details

The PR adds a flag CYPRESS_SKIP_VERIFY to prevent the verification steps from running. These steps are semi-redundant in CI environments, and regularly timeout leading to false negatives in test suites.

Did you open an issue about timeouts so this could be investigated? I'm seeing some other problems in CI environments with cypress verify where the check is uncovering some other issues, so I wonder if it is a good idea to suppress this check.

@joshuajtward
Copy link
Contributor Author

Hi @MikeMcC399! I have not opened an issue about the timeouts, but there has been a lot of discussion in this repo about this issue already (see for example #6082). It has been partially addressed by adding the ability to set a long timeout, but I would like to add the option to suppress the error so that we can:

  1. reduce the false negatives in our CI pipelines
  2. see if suppressing it raises any more informative logs or errors

@MikeMcC399
Copy link
Contributor

@joshuajtward

@joshuajtward
Copy link
Contributor Author

Yes @MikeMcC399, I'm pretty sure something is going awry with the child tasks which interrogate the installed vs expected binaries, but it's pretty hard to narrow it down because it only happens on around 2% of jobs and usually passes on a re-run. I don't think it's a sensible use of resources to put DEBUG=cypress:* in all our CI runs...

For our use cases we don't much care about these integrity checks as we're pretty confident in our CI environment's stability (I think that will be a commonly held sentiment), our jobs are all running on the same container infrastructure, and using the same caches. We don't want to have to implement a retry shell script or other such workarounds, so using a flag to opt out seems like a better way forward for us.

If our use of this flag helps to highlight a root cause or underlying problem I will be happy to raise it as an issue in this repo!

@MikeMcC399
Copy link
Contributor

@joshuajtward

Thanks again for your explanation and background! Which CI provider are you using and which package manager (npm, Yarn or pnpm)?

@joshuajtward
Copy link
Contributor Author

joshuajtward commented Jul 29, 2024

@MikeMcC399 we're using GitHub Actions with a mix of Linux, Windows, and MacOS, and a mix of yarn and npm across a number of repos

@mschile mschile requested a review from AtofStryker July 31, 2024 16:04
@jennifer-shehane jennifer-shehane requested review from MikeMcC399 and jennifer-shehane and removed request for MikeMcC399 and jennifer-shehane August 1, 2024 14:31
@MikeMcC399
Copy link
Contributor

The new environment variable would need to be documented in the table https://docs.cypress.io/guides/references/advanced-installation#Environment-variables so this would mean that a release branch for 13.14.0 (or later if delayed) would need to be created in the docs repo to prepare for release.

image

Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

The documentation aspect needs to be covered.

@joshuajtward
Copy link
Contributor Author

@MikeMcC399 docs PR is here: cypress-io/cypress-documentation#5889

@joshuajtward
Copy link
Contributor Author

@MikeMcC399 @jennifer-shehane is there anything else I can help with to get this moving?

@MikeMcC399
Copy link
Contributor

@joshuajtward

is there anything else I can help with to get this moving?

Yes, you will need to resolve the conflicts in the branch. Unfortunately this can happen as changes are committed to the develop branch.

This is mentioned in the submitter's guide https://github.com/cypress-io/cypress/blob/develop/CONTRIBUTING.md#pull-requests

@joshuajtward
Copy link
Contributor Author

@MikeMcC399 sorry I hadn't seen there were more conflicts. I have updated that now, is there anything else?

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Aug 8, 2024

@joshuajtward

sorry I hadn't seen there were more conflicts. I have updated that now

👍🏻

is there anything else?

CI has one failing test, however that may be CircleCI flake. I expect that @jennifer-shehane will be able to provide more concrete feedback as the Cypress.io team is more familiar with the individual tests than I am. They can also re-run individual tests.

Since I'm only an external contributor, I don't have write privileges to CircleCI, to the repo or to the PR approval process. I can just provide comments.

@jennifer-shehane
Copy link
Member

@joshuajtward Just awaiting @AtofStryker review at this point

cli/lib/tasks/verify.js Outdated Show resolved Hide resolved
cli/lib/tasks/verify.js Outdated Show resolved Hide resolved
cli/lib/tasks/verify.js Show resolved Hide resolved
joshuajtward and others added 3 commits August 9, 2024 14:47
@joshuajtward
Copy link
Contributor Author

Good shout @AtofStryker, that reads more nicely - I've updated it now

@jennifer-shehane
Copy link
Member

@joshuajtward There's a linting error
Screenshot 2024-08-09 at 10 25 04 AM

@joshuajtward
Copy link
Contributor Author

@AtofStryker I have fixed this up and it should be ready for a re-review, thanks!

@AtofStryker
Copy link
Contributor

@joshuajtward Thank you for getting the changes addressed! One last question on my end. I created a binary of your code changes on add6f09. Are you able to download the binary from the link and verify the changes work as expected?

@joshuajtward
Copy link
Contributor Author

joshuajtward commented Aug 15, 2024

@joshuajtward Thank you for getting the changes addressed! One last question on my end. I created a binary of your code changes on add6f09. Are you able to download the binary from the link and verify the changes work as expected?

Hi @AtofStryker, I have installed the linux arm64 binary and run a few tests, it appears to work as expected

Without the flag it works as normal:

No flag

With the flag the verify steps are skipped:

With flag

And if the binary is missing then it fails with an illustrative error:

No binary

Please let me know if there's any other testing you would like me to do!

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Aug 15, 2024

@joshuajtward

I notice that you're running quite an old version of Yarn (v1.18.0) from 2019. Yarn v1.22.22 is the current stable version (see https://classic.yarnpkg.com/en/docs/install).

@joshuajtward
Copy link
Contributor Author

@jennifer-shehane are we all set to merge? 🙏

@jennifer-shehane jennifer-shehane merged commit 482358b into cypress-io:develop Aug 16, 2024
68 checks passed
@joshuajtward joshuajtward deleted the issue-22243 branch August 19, 2024 08:24
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 27, 2024

Released in 13.14.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.14.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Being able to disable cypress verify step
7 participants