Skip to content

Time out browser loads just before the suite load timeout - #2786

Merged
natebosch merged 2 commits into
masterfrom
browser-load-timeout-config
Oct 12, 2026
Merged

Time out browser loads just before the suite load timeout#2786
natebosch merged 2 commits into
masterfrom
browser-load-timeout-config

Conversation

@natebosch

@natebosch natebosch commented Oct 9, 2026

Copy link
Copy Markdown
Member

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.

@natebosch
Time out browser loads just before the suite load timeout
8525569 
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.
@natebosch
natebosch requested a review from a team as a code owner October 9, 2026 00:57
@natebosch
natebosch requested a review from jakemac53 October 9, 2026 00:57
@github-actions

github-actions Bot commented Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check (for example, for non-user-visible changes when the package version is already at -wip).

natebosch
natebosch commented Oct 9, 2026
View reviewed changes

@natebosch natebosch left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't have immediate plans to add narrow timeouts in other platforms, but this is a better design for the browser timeouts, and this will be handy if we end up needing to add narrow timeouts to debug future loading issues.

Comment thread pkgs/test_core/lib/src/runner/platform.dart
Comment on lines +34 to +36
/// more specific error before the suite fails with a generic timeout. The
/// value is only meaningful while [load] is running; callbacks a platform
/// registers during [load] may see a stale value when they run later.

@natebosch natebosch Oct 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For reviewers - this note is here out of an abundance of caution. No platforms would have any reason to read this (nor generally to execute callbacks) past loading and platform plugin APIs are expert facing, not end-user facing. There's no reason to make the zone interactions more complex to protect against misuse.

@jakemac53 jakemac53 Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do they implement this class though? Is this breaking?

@jakemac53 jakemac53 Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

consider making this an extension method if so

@natebosch natebosch Oct 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was written as a static so that it will be safely non-breaking for existing implementations (although I think in practice we could probably make it a base class).

jakemac53
jakemac53 approved these changes Oct 12, 2026
View reviewed changes

@jakemac53 jakemac53 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just the one comment about whether or not adding the new getter is breaking.

@natebosch
natebosch merged commit 30acb69 into master Oct 12, 2026
71 checks passed
@natebosch
natebosch deleted the browser-load-timeout-config branch October 12, 2026 19:42
natebosch added a commit that referenced this pull request Oct 12, 2026
@natebosch
Give browsers until the suite load timeout to connect
1fe53dd 
A browser had 30 seconds to connect, and one that didn't was replaced,
up to 3 attempts, regardless of `suite_load_timeout`. #2786 gave browser
suite loads until shortly before the suite load timeout instead, so the
connection is the last part of loading a browser suite with its own
fixed limit.

Give the connection the same budget: until shortly before the suite load
timeout, or no limit without one. A browser that times out uses the
whole budget, so it isn't replaced. Timeouts were the only failure that
was retried, so instead retry a browser that starts and then exits
before connecting, up to 3 attempts while time remains. A process that
fails to start, such as one with a missing executable, would fail the
same way again and isn't retried. Each failed attempt is described in
the error, including the output of browsers that exited.

Without a suite load timeout a browser that never connects now hangs
the run instead of failing, so add a `suite_load_timeout` to the wasm
and spawn_hybrid integration tests, which run in browsers on CI.
@natebosch natebosch mentioned this pull request Oct 13, 2026
Merged
copybara-service Bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 13, 2026
@natebosch dart-scoped@luci-project-accounts.iam.gserviceaccount.com
[deps] rev dartdoc, ecosystem, http, native, test, tools
070ca6e 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/5eb0f2c..e1859cf):
  e1859cfc  Fri Oct 9 14:53:01 2026 +0200  Sarah Zakarias  Link a bare prefix only when it maps to exactly one library (dart-lang/dartdoc#4295)
  c48f6fb9  Fri Oct 9 14:25:42 2026 +0200  Sarah Zakarias  Resolve prefixes through the analyzer's PrefixElement.scopeLibraries (dart-lang/dartdoc#4294)
  3a6297e2  Wed Oct 7 12:56:14 2026 +0200  Sarah Zakarias  Require analyzer 14.5.0 and bump to 9.0.10 (dart-lang/dartdoc#4292)

ecosystem (https://github.com/dart-lang/ecosystem/compare/16dca7a..622ef61):
  622ef61  Mon Oct 12 00:23:27 2026 -0700  Kevin Moore  fix(firehose): enforce -wip on published versions and tighten changelog checks (dart-lang/ecosystem#467)
  5576a85  Wed Oct 7 09:29:44 2026 +0200  Moritz  Support use-flutter option in groundskeeper workflow (dart-lang/ecosystem#444)

http (https://github.com/dart-lang/http/compare/585d433..23f8574):
  23f8574  Thu Oct 8 23:27:25 2026 +0700  Cong Bang  feat(http2): configurable receive windows and frame size (dart-lang/http#1999)
  18051a6  Thu Oct 8 10:39:38 2026 +0200  Moritz  Enable Flutter in Groundskeeper workflow for Flutter packages (dart-lang/http#2004)

native (https://github.com/dart-lang/native/compare/a2a6179..10515af):
  10515afca  Mon Oct 12 12:21:52 2026 +0200  Daco Harkes  [native_toolchain_c] Do not pass linker flags when compiling static libraries (dart-lang/native#3731)

native is held back at 10515af, before dart-lang/native#3728, because it
breaks the dartdev `dart install` native assets tests. See
dart-lang/native#3743.

test (https://github.com/dart-lang/test/compare/d749b10..30acb69):
  30acb69e  Mon Oct 12 12:42:54 2026 -0700  Nate Bosch  Time out browser loads just before the suite load timeout (dart-lang/test#2786)
  597a0326  Mon Oct 12 12:22:53 2026 -0700  Kevin Moore  Fix misplaced changelog entries and remove no-op health check (dart-lang/test#2789)
  0b6efee8  Mon Oct 12 12:16:29 2026 -0700  Nate Bosch  [checks] Collapse nested subjects in condition descriptions (dart-lang/test#2784)
  17c98493  Mon Oct 12 12:08:23 2026 -0700  Nate Bosch  Follow symlinks in precompiled browser test directories (dart-lang/test#2787)
  1cc813ec  Fri Oct 9 11:59:56 2026 -0700  Nate Bosch  [checks] Join nested collapsed subjects with "that" (dart-lang/test#2783)
  ada7b950  Thu Oct 8 12:03:42 2026 -0700  Nate Bosch  Prepare to publish (dart-lang/test#2780)
  c4b165b3  Wed Oct 7 11:29:08 2026 -0700  Kevin Moore  [checks] Require package:meta ^1.17.0 (dart-lang/test#2779)
  4a363d70  Tue Oct 6 10:46:31 2026 -0700  Nate Bosch  Let Chrome choose its remote debugging port (dart-lang/test#2778)
  465d66ab  Tue Oct 6 09:35:22 2026 -0700  Nate Bosch  Keep Chrome stderr in the browser output when debugging (dart-lang/test#2777)

tools (https://github.com/dart-lang/tools/compare/a66d517..2b54c71):
  2b54c71af  Mon Oct 12 10:16:45 2026 +0200  dart-ecosystem-bot[bot]  [boring] Update BoringSSL to 3fed4f20 (dart-lang/tools#2655)
  2337f995a  Sat Oct 10 03:11:40 2026 +0300  Hasan Armoush  [json_rpc_2] Add strictProtocolChecks parameter to Client (dart-lang/tools#2555)
  c65c69ed1  Fri Oct 9 08:54:25 2026 +0200  Moritz  [boring] Move package:boring to dart-lang/tools (2/4): FFI helpers from package:webcrypto (dart-lang/tools#2615)
  a04e45491  Thu Oct 8 21:07:18 2026 -0700  Kevin Moore  chore: wire package:api_summary and package:string_scanner PR auto-labeler (dart-lang/tools#2652)
  8bdd4b92c  Thu Oct 8 09:20:48 2026 +0200  Morgan :)  [code_builder] Allow built_collection 6.x. (dart-lang/tools#2645)
  c16040d35  Wed Oct 7 17:30:08 2026 -0700  Kevin Moore  [api_summary] Sort primary barrel first and experimental entry points last (v1.2.0) (dart-lang/tools#2619)
  b2d0780db  Wed Oct 7 11:27:16 2026 -0700  Kevin Moore  [stream_channel] Require package:async ^2.6.0 (dart-lang/tools#2644)
  3ab7bf9d0  Wed Oct 7 11:35:27 2026 +0200  Moritz  [boring] Move package:boring to dart-lang/tools (1/4): vendored BoringSSL, bindings and source build (dart-lang/tools#2614)
  29df6348b  Tue Oct 6 15:21:44 2026 -0700  Kevin Moore  feat(coverage): support web coverage reports in test_with_coverage (dart-lang/tools#2448)

Regenerate pkg/{analyzer,analyzer_plugin,analyzer_testing}/api.txt. The
rolled package:api_summary v1.2.0 (dart-lang/tools#2619) sorts the primary
barrel first and other entry points after it, so entries in these files
are reordered. The APIs themselves are unchanged.

Change-Id: Ie8ff19c5a981ea8ecee8939c75457986a947e0a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/563220
Reviewed-by: Samuel Rawlins <[email protected]>
Auto-Submit: Nate Bosch <[email protected]>
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
natebosch added a commit that referenced this pull request Oct 27, 2026
@natebosch
Diagnose browser connection failures and time them out with the suite…
5967927 
… load timeout (#2788)

A wasm integration test failed with "Timed out waiting for Firefox to
connect." and an empty "Browser output:"
(https://github.com/dart-lang/test/actions/runs/37977193562/job/113978908374).
All three connection attempts had failed, but the error only showed the last
attempt's output, and Firefox writes almost nothing to stdout or stderr, so
there was no way to tell how far the browser got.

### Diagnostics

When a browser fails to connect, describe each attempt: how long it waited,
whether the browser process was still running or its exit code, the command
used to start it, its output, and the requests it made to the test server. A
new `BrowserRequestLog` wraps each compiler's server and records the host
page, the resources it loads, and WebSocket upgrades, matched to the browser
manager by the `managerUrl` query parameter or the `Referer` header. Requests
are labeled by file name so the server secret is not printed.

Set Firefox preferences that print page console output and `dump()` calls to
stdout, so errors in the host page show up in the browser output.

### Connect timeout

A browser had 30 seconds to connect, and one that didn't was replaced, up to
3 attempts, regardless of `suite_load_timeout`. Following #2786, give the
connection until shortly before the suite load timeout instead, or no limit
without one. Connecting and then loading the suite are consecutive steps of
loading a suite, so they share that deadline, and loading gets whatever time
connecting left. A browser that times out uses the whole budget, so it isn't
replaced. Timeouts were the only failure that was retried; instead, retry a
browser that starts and then exits before connecting, up to 3 attempts while
time remains. A process that fails to start, such as one with a missing
executable, isn't retried.

Without a suite load timeout a browser that never connects now hangs the run
instead of failing, so add `suite_load_timeout: 2m` to the wasm and
spawn_hybrid integration tests, which run in browsers on CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jakemac53 jakemac53 jakemac53 approved these changes

Assignees

No one assigned

Labels

package:test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Browser suite-load timeout is tied to the test timeout (min 30s) and ignores suite_load_timeout; no way to raise or disable it independently

2 participants

@natebosch @jakemac53