Skip to content

Make the video player poster screenshot test resilient to external video URL failures#2244

Open
leonardomendix wants to merge 3 commits into
mainfrom
test/video-player-route-external-mp4
Open

Make the video player poster screenshot test resilient to external video URL failures#2244
leonardomendix wants to merge 3 commits into
mainfrom
test/video-player-route-external-mp4

Conversation

@leonardomendix
Copy link
Copy Markdown
Collaborator

Pull request type

Test related change (New E2E test, test automation, etc.)


Description

The "renders a poster" screenshot test was flaky in CI because it depended on a third-party CDN (file-examples-com.github.io) that is unreachable from the runner. When the video fails to load, the widget shows an error overlay and hides native controls, producing a screenshot that doesn't match the baseline.

Two fixes:

  1. Route interception — page.route("**/*.mp4") intercepts the CDN request before the page reloads and fulfills it with a local 489-byte stub MP4 (e2e/fixtures/stub.mp4). The stub has a valid ftyp+moov structure so Chrome can parse metadata without fetching any actual video data. The widget is still configured with the external URL — only the network call is replaced.
  2. Condition-based wait — replaced the unreliable Image() preload trick with a loadedmetadata event listener on the video element itself. The old approach only waited for the poster image bytes to decode, not for the

@leonardomendix leonardomendix requested a review from a team as a code owner June 2, 2026 14:33
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js Replace flaky CDN-dependent poster screenshot test with route interception + metadata-ready guard
packages/pluggableWidgets/video-player-web/e2e/fixtures/stub.mp4 New binary: minimal valid MP4 stub served by the route interceptor

Skipped (out of scope): dist/, pnpm-lock.yaml

CI check results could not be retrieved (command required approval). No CHANGELOG entry required — this is a test-only change with no runtime/XML/behavior impact.


Findings

⚠️ Low — afterEach session logout missing from the "External video" describe block

File: packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js lines 109–184
Note: The "External video" describe block (and its nested "Video aspect ratio" block) has no afterEach logout. This pre-exists the PR, but the new "renders a poster" test now calls page.reload() mid-test, which is an additional session action that makes the missing cleanup more noticeable. Under the repo's E2E rules every describe block needs:

test.afterEach("Cleanup session", async ({ page }) => {
    await page.evaluate(() => window.mx.session.logout());
});

Without it, CI can exhaust Mendix's 5-session limit across parallel workers.


⚠️ Low — Route pattern "**/*.mp4" is broader than intended

File: packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js line 117
Note: "**/*.mp4" will intercept every .mp4 request on the page, including any future video sources added to the test app. If another test on the same page loads a different video, it will silently receive the stub. A URL-specific pattern is safer:

await page.route("**/file-examples-com.github.io/**/*.mp4", route => ...);
// or match the exact URL the widget uses:
await page.route(/file-examples-com\.github\.io/, route => ...);

⚠️ Low — page.reload() without waitForLoadState after reload

File: packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js line 125
Note: page.reload() returns once navigation completes but the Mendix runtime may not have re-rendered the widget yet. Per repo E2E guidelines, follow reloads with waitForLoadState("networkidle") to avoid racing the widget initialisation before the locator assertions:

await page.reload();
await page.waitForLoadState("networkidle");

Positives

  • The route-interception approach correctly eliminates the external CDN dependency without changing the widget's configured URL — the test still exercises the real external-URL code path.
  • The readyState >= 1 check with a pre-existing-error guard is a precise, well-commented wait condition that avoids both a busy-poll and an infinite hang.
  • Committing a minimal valid MP4 fixture (stub.mp4) rather than a URL or a huge media file is the right call for hermetic E2E tests.
  • The PR description clearly explains both the root cause (CDN unreachable in CI) and the two-part fix, making it easy to verify intent against the diff.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js Rewrote "renders a poster" test: added route interception for mp4, replaced Image() preload with waitForFunction polling on video element state
packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js-snapshots/videoPlayerExternalPoster-chromium-darwin.png New macOS screenshot baseline
packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js-snapshots/videoPlayerExternalPoster-chromium-linux.png Updated Linux screenshot baseline
packages/pluggableWidgets/video-player-web/e2e/fixtures/stub.mp4 New minimal stub MP4 for route interception

Skipped (out of scope): dist/, pnpm-lock.yaml

CI checks: could not be fetched from this environment — manual CI verification recommended.


Findings

⚠️ Low — Missing page.waitForLoadState("networkidle") after page.goto

File: packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js line 121
Note: The page.goto("/p/external") call doesn't wait for networkidle before interacting with the page. Other beforeEach blocks in this same file all use waitForLoadState("networkidle") (implied by the shared test infrastructure). While the waitForFunction polling below effectively guards against a race in this specific test, adding waitForLoadState here aligns the test with the rest of the file's patterns and makes the intent more explicit.
Fix:

await page.goto("/p/external");
await page.waitForLoadState("networkidle");

⚠️ Low — testInfo.file.replace(...) is fragile if the test file is renamed

File: packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js line 113
Note: The path to stub.mp4 is derived by string-replacing the spec filename in testInfo.file. This works today but breaks silently if the spec file is ever renamed or if the test is moved to a different describe/file. Using path.join(path.dirname(testInfo.file), "fixtures", "stub.mp4") is more robust — it doesn't depend on the literal filename.
Fix:

import path from "path";
// ...
const stubPath = path.join(path.dirname(testInfo.file), "fixtures", "stub.mp4");

⚠️ Low — waitForFunction selector doesn't use .mx-name-* — minor but inconsistent

File: packages/pluggableWidgets/video-player-web/e2e/VideoPlayer.spec.js line 133
Note: The document.querySelector(".widget-video-player video") inside waitForFunction uses a widget CSS class rather than an .mx-name-* locator (the preferred stable selector per E2E guidelines). This is minor since it runs inside a page.evaluate-style context where Playwright locators aren't available, but worth noting if the widget name is accessible. Not a blocker.


Positives

  • The route interception approach is the correct Playwright pattern for preventing flaky tests caused by unreachable CDN dependencies — the widget still uses the real external URL, only the network call is replaced, which accurately reflects the production configuration.
  • Replacing the event-listener–based Image() promise with a waitForFunction poll is a meaningful reliability improvement. The old approach had a documented race; polling on readyState/networkState/error is the right model for <video> element state.
  • Committing both darwin and linux screenshot baselines is correct — toHaveScreenshot requires committed baselines to work in CI.
  • The el.error !== null fast-fail guard inside waitForFunction means the test will time out quickly with a clear signal rather than hanging for 8 seconds if route interception fails.

@leonardomendix leonardomendix enabled auto-merge June 3, 2026 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants