Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,7 @@ CLAUDE.md
vtk-js/
.ohif-downstream/
auth.json

# Vitest browser mode run artifacts
.vitest-attachments/
tests/vitest-browser/__screenshots__/
11 changes: 11 additions & 0 deletions tests/dicomImageLoaderWADOURI.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ import {
visitExample,
screenShotPaths,
waitForImageRendered,
retryRemoteFixtures,
} from './utils/index';
import { dicomDimensions } from '../packages/dicomImageLoader/examples/dicomImageLoaderWADOURI/dicomDimensions';

test.beforeEach(async ({ page }) => {
// Every image in this example is fetched over HTTP from
// raw.githubusercontent.com, which intermittently rate-limits/drops requests
// under the parallel workers on the self-hosted runner. Retry those fetches
// with backoff so a single dropped response doesn't fail the image load.
await retryRemoteFixtures(page);
await visitExample(page, 'dicomImageLoaderWADOURI');
});

Expand Down Expand Up @@ -86,6 +92,11 @@ async function selectImageAndWaitForRender(page: Page, imagePath: string) {
() => page.locator('#imageSelector').selectOption(imagePath),
{
expectedImageId: getExpectedWadoImageId(imagePath),
// Larger budget than the 30s default: the large TG18 1k/2k images can
// legitimately take a while to download+decode on the self-hosted
// runner, and retryRemoteFixtures may add a few seconds of backoff on a
// transient GitHub-raw failure.
timeout: 60000,
}
);

Expand Down
11 changes: 11 additions & 0 deletions tests/genericViewport/genericDicomImageLoaderWADOURI.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
createExampleUrl,
screenShotPaths,
waitForImageRendered,
retryRemoteFixtures,
} from '../utils/index';
import { dicomDimensions } from '../../packages/dicomImageLoader/examples/dicomImageLoaderWADOURI/dicomDimensions';

Expand Down Expand Up @@ -74,6 +75,11 @@ async function selectImageAndWaitForRender(page: Page, imagePath: string) {
() => page.locator('#imageSelector').selectOption(imagePath),
{
expectedImageId: getExpectedWadoImageId(imagePath),
// Larger budget than the 30s default: the large TG18 1k/2k images can
// legitimately take a while to download+decode on the self-hosted
// runner, and retryRemoteFixtures may add a few seconds of backoff on a
// transient GitHub-raw failure.
timeout: 60000,
}
);

Expand All @@ -84,6 +90,11 @@ async function selectImageAndWaitForRender(page: Page, imagePath: string) {
}

test.beforeEach(async ({ page }) => {
// Every image in this example is fetched over HTTP from
// raw.githubusercontent.com, which intermittently rate-limits/drops requests
// under the parallel workers on the self-hosted runner. Retry those fetches
// with backoff so a single dropped response doesn't fail the image load.
await retryRemoteFixtures(page);
const url = createExampleUrl(EXAMPLE + '.html');
url.searchParams.set('type', 'next');
await page.goto(url.toString());
Expand Down
1 change: 1 addition & 0 deletions tests/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export { createExampleUrl } from './createExampleUrl';
export { getSegmentationActorClassNames } from './getSegmentationActorClassNames';
export { expectGenericViewportRuntime } from './expectGenericViewportRuntime';
export { waitForImageRendered } from './waitForImageRendered';
export { retryRemoteFixtures } from './retryRemoteFixtures';
export {
setupRenderTracking,
waitForViewportsRendered,
Expand Down
83 changes: 83 additions & 0 deletions tests/utils/retryRemoteFixtures.ts
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;
Comment on lines +38 to +42

Copy link
Copy Markdown

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 waitForImageRendered 60s gate. The browser's XHR is blocked inside the route handler until fulfill/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 perAttemptTimeoutMs to ~12s (worst case 51.5s < 60s) or documenting the relationship between these two values.

💡 Suggested fix: reduce default per-attempt timeout
   /** Per-attempt fetch timeout in ms. Default 20000. */
-  perAttemptTimeoutMs?: number;
+  perAttemptTimeoutMs?: number;
   const {
     attempts = 4,
     hosts = DEFAULT_HOSTS,
-    perAttemptTimeoutMs = 20000,
+    perAttemptTimeoutMs = 12000,
   } = options;

Also applies to: 68-71

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/utils/retryRemoteFixtures.ts` around lines 38 - 42, The retry fixture
defaults in retryRemoteFixtures are allowing a worst-case duration that exceeds
the 60s render gate, so adjust the defaults in the options destructuring and
related helper usage to keep the route handler under the render timeout. Update
the perAttemptTimeoutMs default in retryRemoteFixtures (and any linked call
sites/specs that rely on it) so the total retry window stays below
waitForImageRendered, or add a clear comment/docstring in retryRemoteFixtures
explaining the required relationship between attempts, perAttemptTimeoutMs, and
the 60s timeout.


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();
}
}
);
}
Loading
Loading