Skip to content

Timeout browser suite loads - #2080

Merged
natebosch merged 11 commits into
masterfrom
browser-verbose-logs
Aug 15, 2023
Merged

natebosch merged 11 commits into
masterfrom
browser-verbose-logs

Conversation

@natebosch

@natebosch natebosch commented Aug 11, 2023 •

Copy link
Copy Markdown
Member

The current timeout applies to starting the process and connecting back
from the host. Add a timeout which applies on each suite as it is
loaded.

Change the chrome argument to log to stderr instead of stdout. The
latter seems to have no effect.

Eagerly decode and split lines from the browser output instead of
storing the bytes to avoid mixing lines between the two outputs. In most
cases this shouldn't matter, I have not seen output on stdout.

Use --v=0 instead of --v=1 since the 0 level includes console
output, and the extra browser level output with higher verbosity does
not look useful.

The current timeout applies to starting the process and connecting back
from the host. Add a timeout which applies on each suite as it is
loaded.

Change the chrome argument to log to `stderr` instead of `stdout`. The
latter seems to have no effect.

Eagerly decode and split lines from the browser output instead of
storing the bytes to avoid mixing lines between the two outputs. In most
cases this shouldn't matter, I have not seen output on `stdout`.

Use `--v=0` instead if `--v=1` since the 0 level includes console
output, and the extra browser level output with higher verbosity does
not look useful.
@natebosch
natebosch requested a review from jakemac53 August 11, 2023 19:38
@natebosch

Copy link
Copy Markdown
Member Author

@jakemac53 - I switched this from configurable per suite to hardcoded at 30 seconds. Do you have any concerns about that? This timeout does include waiting for main to complete for the test suite.

The reason I hardcoded it is because we have some tests where the configured timeout seems to be too low for a suite and the load was timing out.

If a suite sets a timeout too low, extend it back to 30 seconds for the
startup. This will still allow slow `main` to compensate by setting a
longer timeout or a scaling factor.
@natebosch

natebosch commented Aug 11, 2023 •

Copy link
Copy Markdown
Member Author

I think it may work to leave it configurable so there is an escape hatch for slow main, but have a lower bound at 30 seconds if the suite sets the timeout too low. WDYT?

@jakemac53

Copy link
Copy Markdown
Contributor

A lower bound but configurable is probably fine.

@natebosch
natebosch merged commit 46cf4de into master Aug 15, 2023
@natebosch
natebosch deleted the browser-verbose-logs branch August 15, 2023 16:35
natebosch added a commit that referenced this pull request Oct 12, 2026
Fixes #2691

The browser platform gave up waiting for a browser to load a compiled
suite after the test timeout, or 30 seconds if that was longer. This
timeout was added in #2080, before `suite_load_timeout` existed, to get
better diagnostics for browser loads that never completed. The test
timeout has nothing to do with loading, and `suite_load_timeout` had no
effect on this limit, so the only way to give a slow browser more time
was to raise the timeout for every test.

Start a stopwatch in the load suite's body, right after the invoker
starts the load test's timeout, and expose the time left before that
timeout through a static `PlatformPlugin.remainingLoadTime` getter that
platforms can read during `load`. It reads a zone value set by the load
suite, so platforms need no new imports, and it is static so classes
implementing `PlatformPlugin` don't need to change. It is `null` when
loading can't time out because the suite load timeout is `none` or
timeouts are ignored. The value is only meaningful during `load`;
callbacks registered while loading run in the same zone and may see a
stale value later.

Have the browser platform give up 1 second before the remaining load
time runs out, so its error, which includes the browser output, wins
over the generic timeout on the load test. Retries in the loader share
the same budget. When there is no remaining load time, don't time out
the browser, matching how other platforms load suites without a suite
load timeout. Configuring `suite_load_timeout` restores a limit along
with the more detailed error.

Bump test_core to 0.6.22-wip for the new API and pin it from test.
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.

2 participants