Yicong-Huang commented on code in PR #7623:
URL: https://github.com/apache/texera/pull/7623#discussion_r3782721212
##########
frontend/vitest.config.ts:
##########
@@ -34,6 +34,16 @@ export default defineConfig({
// which Angular's `fakeAsync` requires. Karma+Jasmine installed this
// implicitly; the @angular/build:unit-test path doesn't.
setupFiles: ["src/test-zone-setup.ts"],
+ // Vitest defaults (5s per test, 10s per hook) are too tight for the
+ // macOS runners, which stall for seconds at a time under load: the same
+ // spec file that takes 240ms on ubuntu-latest has been observed taking
+ // 11.7s on macos-latest in the same commit's matrix. The stall lands on
+ // whichever test happens to be running, so raising the ceiling is the
+ // only fix that isn't whack-a-mole — three different specs have gone
+ // red this way. A test that legitimately needs >30s is broken, and the
+ // job's own timeout still bounds a true hang. See apache/texera#6073.
Review Comment:
Nothing backs this reassurance: the `frontend` job sets no `timeout-minutes`
(the only one is on the "Install dependency" step, `build.yml:135`), so the
bound is GitHub's implicit 6h default — billed 10x on macOS. Literally true,
misleading in the way that matters. Two jobs down, `amber-integration`
(`build.yml:366-370`) sets `timeout-minutes: 25` rather than rely on the cap.
Adding the same key here (green runs are 8-13 min) makes the sentence carry its
weight.
##########
.github/workflows/build.yml:
##########
@@ -87,6 +87,12 @@ jobs:
if: ${{ inputs.run_frontend }}
runs-on: ${{ matrix.os }}
strategy:
+ # An OS-specific failure should not cancel the other two legs: with the
+ # default fail-fast the surviving jobs report "The operation was
+ # canceled" and the run no longer says whether the failure reproduces
+ # off that OS — exactly the evidence needed to tell a runner flake from
+ # a real break. Every other multi-leg matrix here already opts out.
Review Comment:
Refuted twice in this same file: `amber-integration` (`:378-380`, `os:
[ubuntu-latest, macos-latest]`) and `pyamber` (`:1038-1040`, three Python
versions) are both multi-leg with no `fail-fast` key. The set that actually
opts out is `platform`, `platform-integration`, `agent-service`, and `infra` —
naming those keeps it true. (Aside for a follow-up: `amber-integration` runs
macOS beside ubuntu with no opt-out, so this cancellation still applies there.)
##########
frontend/vitest.browser.config.ts:
##########
@@ -64,6 +64,11 @@ export default defineConfig({
// browser-mode the runtime has neither, so we install the `buffer` npm
// package as a shim).
setupFiles: ["src/browser-buffer-polyfill.ts", "src/test-zone-setup.ts"],
+ // Same runner-stall headroom as the jsdom config (vitest.config.ts):
+ // driving a real Chromium through playwright is strictly slower than
+ // jsdom, so these specs need at least as much slack. See #6073.
+ testTimeout: 30_000,
+ hookTimeout: 30_000,
Review Comment:
Vitest resolves timeout defaults from `browser.enabled`, which this config
sets `true`. In the pinned 4.1.10
(`vitest/dist/chunks/coverage.DM_a_rWm.js:538-539`): `hookTimeout ??=
browser.enabled ? 3e4 : 1e4`, `testTimeout ??= browser.enabled ? 15e3 : 5e3`.
So the real defaults here are already 15s/30s — `hookTimeout: 30_000` just
restates the default, and `testTimeout` is a 2x bump, not 6x. That also makes
TESTING.md's "raised from the 5s/10s defaults" wrong for the browser leg (right
for jsdom). Suggest dropping this line and scoping the doc row per-config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
