-
Notifications
You must be signed in to change notification settings - Fork 506
test(vitest-browser): state-based integration suites for GenericViewport and tools #2792
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import type { APIResponse, Page, Route } from '@playwright/test'; | ||
|
|
||
| const DEFAULT_HOSTS = [/raw\.githubusercontent\.com/]; | ||
|
|
||
| interface RetryRemoteFixturesOptions { | ||
| /** Total attempts (including the first) before giving up. Default 4. */ | ||
| attempts?: number; | ||
| /** Host patterns to intercept. Default: raw.githubusercontent.com. */ | ||
| hosts?: RegExp[]; | ||
| /** Per-attempt fetch timeout in ms. Default 20000. */ | ||
| perAttemptTimeoutMs?: number; | ||
| } | ||
|
|
||
| /** | ||
| * DICOM fixtures for the loader examples are fetched over HTTP from | ||
| * raw.githubusercontent.com — most images from the cornerstone3D repo, the | ||
| * TG18 set from the external OHIF/viewer-testdata repo. Under the parallel | ||
| * Playwright workers on the self-hosted runner, GitHub raw intermittently | ||
| * rate-limits (429) or drops these requests. The loader issues a single | ||
| * XMLHttpRequest per image with no retry (see | ||
| * packages/dicomImageLoader/src/imageLoader/internal/xhrRequest.ts), so one | ||
| * bad response fails the whole image load and surfaces as a flaky | ||
| * `waitForImageRendered` timeout. | ||
| * | ||
| * Intercept those requests and retry them with exponential backoff from the | ||
| * Node side via `route.fetch`, then replay the successful response to the | ||
| * browser. Retries happen off the browser's single-shot XHR, the product code | ||
| * is untouched, and the example's public URLs are left as-is so the deployed | ||
| * docs demo is unaffected. Range requests are preserved because `route.fetch` | ||
| * forwards the original request (headers included). | ||
| * | ||
| * Install in a spec's `beforeEach` BEFORE navigating to the example. | ||
| */ | ||
| export async function retryRemoteFixtures( | ||
| page: Page, | ||
| options: RetryRemoteFixturesOptions = {} | ||
| ): Promise<void> { | ||
| const { | ||
| attempts = 4, | ||
| hosts = DEFAULT_HOSTS, | ||
| perAttemptTimeoutMs = 20000, | ||
| } = options; | ||
|
|
||
| await page.route( | ||
| (url) => hosts.some((host) => host.test(url.href)), | ||
| async (route: Route) => { | ||
| let lastResponse: APIResponse | undefined; | ||
|
|
||
| for (let attempt = 0; attempt < attempts; attempt++) { | ||
| try { | ||
| const response = await route.fetch({ timeout: perAttemptTimeoutMs }); | ||
| const status = response.status(); | ||
|
|
||
| // Only 429 and 5xx are transient; anything else (2xx, 3xx, 4xx | ||
| // other than 429) is a real answer we should replay immediately. | ||
| if (status !== 429 && status < 500) { | ||
| await route.fulfill({ response }); | ||
| return; | ||
| } | ||
|
|
||
| lastResponse = response; | ||
| } catch { | ||
| // Network error / timeout: fall through to backoff and retry. | ||
| } | ||
|
|
||
| // Backoff after every failed attempt except when we are about to give | ||
| // up, so the caller's render-gate budget is not spent needlessly. | ||
| if (attempt < attempts - 1) { | ||
| const backoffMs = 500 * 2 ** attempt; | ||
| await new Promise((resolve) => setTimeout(resolve, backoffMs)); | ||
| } | ||
| } | ||
|
|
||
| // Retries exhausted. Replay the last transient response if we have one, | ||
| // otherwise let the request proceed so the real network error surfaces. | ||
| if (lastResponse) { | ||
| await route.fulfill({ response: lastResponse }); | ||
| } else { | ||
| await route.continue(); | ||
| } | ||
| } | ||
| ); | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Worst-case retry duration (83.5s) exceeds the 60s render timeout in both spec files.
With defaults (4 attempts × 20s timeout + 500+1000+2000ms backoff = 83.5s), if every attempt times out, the route handler alone runs 23.5s past the
waitForImageRendered60s gate. The browser's XHR is blocked inside the route handler untilfulfill/continue, so the render timeout fires while retries are still in-flight, producing a confusing timeout failure rather than a clear network error.Consider reducing
perAttemptTimeoutMsto ~12s (worst case 51.5s < 60s) or documenting the relationship between these two values.💡 Suggested fix: reduce default per-attempt timeout
const { attempts = 4, hosts = DEFAULT_HOSTS, - perAttemptTimeoutMs = 20000, + perAttemptTimeoutMs = 12000, } = options;Also applies to: 68-71
🤖 Prompt for AI Agents