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: Added configuration parameter that controls the amount of connection attempts to the browser #25848

Merged
merged 14 commits into from
Mar 2, 2023

Conversation

Malvitus
Copy link
Contributor

Additional details

Currently, there is a hardcoded limit of 62 attempts until a connection to the browser is considered fails. For my usecase, this timeout is sometimes to low and i can't change it.

I added a config value and made sure that it is used at the corresponding places when connections to browsers is attempted.

Steps to test

  1. Try to start a browser without changing the config. Everything should work just fine
  2. Set connectRetryThreshold to an absurd low value (like 5). Connection should fail due to timeout

How has the user experience changed?

In the default usecase, Experience did not change. However when users experience connection timeouts, User can adjust the config value to stabilize their tests.

PR Tasks

Other

As this is my first PR for cypress, i am unsure if I did everything right. Should something be amiss, please contact me and I will try my best to solve the issues.

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2023

CLA assistant check
All committers have signed the CLA.

@cypress
Copy link

cypress bot commented Feb 16, 2023

35 flaky tests on run #44163 ↗︎

0 26802 1271 0 Flakiness 35

Details:

feat: Added configuration parameter that controls the amount of connection attem...
Project: cypress Commit: 4fd816a7fe
Status: Passed Duration: 19:47 💡
Started: Feb 16, 2023 12:24 PM Ended: Feb 16, 2023 12:44 PM
Flakiness  e2e/origin/cookie_behavior.cy.ts • 4 flaky tests • 5x-driver-electron

View Output Video

Test
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test
network stubbing > intercepting request > can delay and throttle a StaticResponse
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry
Flakiness  create-from-component.cy.ts • 2 flaky tests • app-e2e

View Output Video

Test
... > runs generated spec Screenshot
... > runs generated spec Screenshot

The first 5 flaky specs are shown, see all 14 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

@Malvitus Thanks for opening a PR.

Instead of adding a new configuration option, could you use an environment variable instead? Something like CYPRESS_BROWSER_CONNECTION_RETRIES. Since the vast majority of users won't need to adjust this value there's no need to put it in config.

@Malvitus
Copy link
Contributor Author

Malvitus commented Feb 17, 2023

Sure, i can change it to an environment variable. Should I open a new PR then? I guess when using an environment variable, i can circumvent a lot of parameter passing and use it directly at the places that are relevant, so I would refactor my PR anyway

@Malvitus
Copy link
Contributor Author

@flotwig
I completed the changes and kept them in this PR just in case. I did not modify the changelog yet as im unsure when this change can be released. I also adjusted my Pr in the documentation repo

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

This looks good @Malvitus. Can you clarify your use case on why you want to add this? Why is it taking more than a minute for your browser to open?

@mjhenkes mjhenkes assigned mjhenkes and unassigned flotwig Feb 28, 2023
Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
@Malvitus
Copy link
Contributor Author

Malvitus commented Mar 1, 2023

@flotwig

I confess that my setup is not that straight forward. For once, my server executing the tests is not that beefy, so execution of anything is not that fast (which I don't mind). The other factor is that I have different browser versions i want to use in my Cypress tests on a network share (as I don't want them to clutter the machine). So depending on the available bandwidth, starting a browser can take longer than expected. Again this is fine for me as I don't have that many tests. Also its not the case that the browsers never start in time, just that they sometimes take a bit longer, which introduces a flakyness to my tests. So after I saw that the connection timeout is hardcoded, I thought it might be a good idea to introduce a config option there so that users can adjust the timeout up or down if needed.

@mjhenkes
Copy link
Member

mjhenkes commented Mar 1, 2023

@Malvitus could you add a changelog entry here: https://github.com/Malvitus/cypress/blob/develop/cli/CHANGELOG.md

**Features:**

 - <Insert change details>. Addressed in [#25848](https://github.com/cypress-io/cypress/pull/25848).

@Malvitus
Copy link
Contributor Author

Malvitus commented Mar 2, 2023

@mjhenkes Done. Do you need anything else?

@mjhenkes
Copy link
Member

mjhenkes commented Mar 2, 2023

@Malvitus, looks good! Thanks for the contribution. I'll work on getting this merged today.

cli/CHANGELOG.md Outdated Show resolved Hide resolved
cli/CHANGELOG.md Outdated Show resolved Hide resolved
cli/CHANGELOG.md Outdated Show resolved Hide resolved
@mjhenkes mjhenkes merged commit fcac397 into cypress-io:develop Mar 2, 2023
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.

None yet

5 participants