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

Do not apply browser timeout option as default navigation timeout #864

Closed
Tracked by #854
ka3de opened this issue Apr 27, 2023 · 1 comment · Fixed by #866
Closed
Tracked by #854

Do not apply browser timeout option as default navigation timeout #864

ka3de opened this issue Apr 27, 2023 · 1 comment · Fixed by #866
Assignees
Labels
breaking PRs that need to be mentioned in the breaking changes section of the release notes
Milestone

Comments

@ka3de
Copy link
Collaborator

ka3de commented Apr 27, 2023

Currently the timeout option defined at the browser level applies to two different functionalities:

  • Sets the TO to apply for some "browser initialization" actions, such as TO when launching the browser instance.
  • Sets the default navigation TO. This can be overwritten at the browserContext and page levels.

This double purpose is wrong, as a the use cases are quite different and a single value for both does not make sense. At the same time, though, we still believe that defining a TO for the browser initialization actions provides value and is required. Taking this into account our decisions have been:

  • The browser level timeout option will apply only for the browser initialization actions. Not for the default navigation TO.
  • The browser level timeout option will be moved to be defined as an ENV variable, instead of being defined at the script level.
  • The default navigation TO will be set by our own implementation with an initial sensible value. This value will be able to be overwritten at the browserContext and page levels, as it's currently possible.

Related: #856.

@ka3de ka3de added the breaking PRs that need to be mentioned in the breaking changes section of the release notes label Apr 27, 2023
@ka3de ka3de self-assigned this Apr 27, 2023
@ankur22 ankur22 added this to the v0.10.0 milestone Apr 28, 2023
@ka3de
Copy link
Collaborator Author

ka3de commented May 2, 2023

There was a misunderstanding in the analysis that resulted in this issue based on the documentation and the bug described in #865 , which, considering that most tests start with a navigation, behaves in a misleading manner which makes it look like it's the navigation TO the one that makes the test fail, but instead is the incorrectly set browser process TO.

Currently the browser.timeout is not set as the default navigation TO, instead, the default navigation TO applied to page and frame is inherited from browserContext's timeoutSettings, which are initialized to nil in its constructor, making it effectively use the default timeout of 30s.

Therefore, no exact fix is required for this, and instead fixing issue #865 will achieve the desired behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants