Skip to content

test(timeslider): port legacy mocha specs to Playwright, retire orphaned suite#7949

Merged
JohnMcLear merged 3 commits into
ether:developfrom
JohnMcLear:test/port-timeslider-frontend-tests
Jun 12, 2026
Merged

test(timeslider): port legacy mocha specs to Playwright, retire orphaned suite#7949
JohnMcLear merged 3 commits into
ether:developfrom
JohnMcLear:test/port-timeslider-frontend-tests

Conversation

@JohnMcLear

Copy link
Copy Markdown
Member

Close the timeslider CI coverage gap

Follow-up to #7948. While fixing #7946 we found that the legacy mocha suite (src/tests/frontend/specs/) is run by no CI workflowfrontend-tests.yml only runs Playwright (tests/frontend-new/specs/). That's why a regression in the in-pad history UI (#7659) reached a release: the timeslider's only coverage lived in dead specs, and the modern Playwright specs drive the isolated ?embed=1 iframe and never exercise the outer in-pad UI users actually interact with.

This ports the still-meaningful timeslider cases to Playwright, re-targeted at the real in-pad UI (outer history banner / slider / export links that pad_mode.ts drives), and deletes the orphaned originals.

New specs

New (frontend-new) Ported from (frontend, deleted) Asserts
timeslider_revision_labels.spec.ts timeslider_labels.js #history-banner shows Version N + a valid (non-NaN) date & timer, and both update when scrubbing to rev 0
timeslider_export_links.spec.ts timeslider_numeric_padID.js + the "checks the export url" case of timeslider_revisions.js outer export hrefs target /p/<pad>/<rev>/export/<type> for the viewed revision — incl. a numeric pad id — and follow the slider to rev 0
timeslider_deeplink.spec.ts the "jumps to a revision given in the url" case of timeslider_revisions.js a #rev/N hash, and the legacy #N shortlink, boot straight into history mode at that revision (bootstrapFromHash)

The star-marker case of timeslider_revisions.js is already covered by timeslider_saved_revisions.spec.ts in #7948.

Verification

All new specs pass on Chromium and Firefox locally; type-check clean. These now run in PR CI, so the in-pad history banner, export-link rewriting, and deep-link entry are gated against regression.

🤖 Generated with Claude Code

…ned suite

The legacy src/tests/frontend/specs/ mocha suite is run by no CI workflow,
so its timeslider coverage was dead — a regression in the in-pad history UI
(ether#7659/ether#7946) sailed through CI. Port the still-meaningful cases to
frontend-new Playwright specs, re-targeted at the real in-pad UI (outer
banner / slider / export links that pad_mode.ts drives) rather than the
isolated ?embed=1 iframe the existing specs use:

- timeslider_revision_labels.spec.ts (from timeslider_labels.js): the
  #history-banner shows 'Version N' + a valid (non-NaN) date and timer, and
  both update when scrubbing to revision 0.
- timeslider_export_links.spec.ts (from timeslider_numeric_padID.js and the
  'checks the export url' case of timeslider_revisions.js): the outer export
  hrefs target /p/<pad>/<rev>/export/<type> for the viewed revision, including
  a numeric pad id, and follow the slider to revision 0.
- timeslider_deeplink.spec.ts (from the 'jumps to a revision given in the url'
  case): a #rev/N hash — and the legacy #N shortlink form — boots straight
  into history mode at that revision.

Delete the three now-ported legacy specs. Verified passing on Chromium and
Firefox. The star-marker case of timeslider_revisions.js is already covered by
timeslider_saved_revisions.spec.ts (ether#7948).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

Grey Divider


Remediation recommended

1. Locale-fragile banner assertions ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new timeslider Playwright specs assert English-only strings (e.g., Version N, Saved …) and
parse localized UI text with Date(), which will fail when the browser locale is not English or
uses a non-JS-Date-parseable format. These files do not pin locale, unlike other existing
Playwright specs that do so when asserting English text.
Code

src/tests/frontend-new/specs/timeslider_revision_labels.spec.ts[R24-67]

+  const goToRevision = async (page: Page, rev: number) => {
+    await page.locator('#history-slider-input').evaluate((el, value) => {
+      (el as HTMLInputElement).value = String(value);
+      el.dispatchEvent(new Event('input', {bubbles: true}));
+    }, rev);
+    await expect(page.locator('#history-banner-rev')).toHaveText(`Version ${rev}`, {timeout: 15000});
+  };
+
+  // "Saved June 12, 2026" -> a parseable Date (the banner mirrors the
+  // timeslider.saved l10n string "Saved {{month}} {{day}}, {{year}}").
+  const parsedBannerDate = async (page: Page) => await page.locator('#history-banner-date').evaluate(
+      (el) => new Date((el.textContent || '').replace(/^Saved\s+/i, '')).getTime());
+
+  test('shows Version label and a valid date that update while scrubbing', async function ({page}) {
+    await goToNewPad(page);
+    await clearPadContent(page);
+
+    // Produce a few revisions.
+    await writeToPad(page, 'Alpha ');
+    await page.waitForTimeout(400);
+    await writeToPad(page, 'Beta ');
+    await page.waitForTimeout(400);
+    await writeToPad(page, 'Gamma ');
+    await page.waitForTimeout(800);
+
+    await enterHistoryMode(page);
+
+    // On entry the slider sits at the latest revision; the banner must show a
+    // non-empty "Version N" label and a non-NaN date. Wait for pad_mode to sync
+    // the slider max from the embedded BroadcastSlider before reading it.
+    await expect.poll(
+        async () => await page.locator('#history-slider-input').evaluate(
+            (el) => Number((el as HTMLInputElement).max)),
+        {timeout: 15000}).toBeGreaterThan(0);
+    const maxRev = await page.locator('#history-slider-input').evaluate(
+        (el) => Number((el as HTMLInputElement).max));
+    expect(maxRev).toBeGreaterThan(0);
+
+    await expect(page.locator('#history-banner-rev')).toHaveText(`Version ${maxRev}`);
+    const dateLast = await parsedBannerDate(page);
+    expect(Number.isNaN(dateLast)).toBe(false);
+    // The mirrored timer must also be a real, non-NaN datetime.
+    const timerLast = await page.locator('#history-timer').textContent();
+    expect(Number.isNaN(new Date(timerLast || '').getTime())).toBe(false);
Evidence
The UI strings under test are explicitly localized (via html10n) and vary across locale bundles; the
new tests hard-code English text and rely on JS Date() parsing of localized formats, so they are
not deterministic without pinning locale or using locale-independent checks.

src/tests/frontend-new/specs/timeslider_revision_labels.spec.ts[24-67]
src/static/js/broadcast_slider.ts[106-121]
src/static/js/broadcast.ts[294-340]
src/locales/en.json[394-412]
src/locales/de.json[232-243]
src/locales/ja.json[119-129]
src/tests/frontend-new/specs/a11y_dialogs.spec.ts[1-9]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New specs assert exact English UI strings ("Version N", "Saved …") and parse the localized timer/banner text via `new Date(...)`. This breaks when Playwright runs with a non-English locale or with a locale whose `timeslider.dateformat` / `timeslider.saved` strings are not parseable by JS `Date`.
## Issue Context
- `#history-banner-rev` mirrors the inner timeslider `#revision_label`, which is set via `html10n.get('timeslider.version', ...)`.
- `#history-banner-date` mirrors `#revision_date`, which is localized via `html10n.get('timeslider.saved', ...)` and is not guaranteed to start with "Saved ".
- `#history-timer` mirrors `#timer`, which uses `html10n.get('timeslider.dateformat', ...)` and varies significantly by locale.
## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_revision_labels.spec.ts[24-67]
- src/tests/frontend-new/specs/timeslider_export_links.spec.ts[40-46]
- src/tests/frontend-new/specs/timeslider_deeplink.spec.ts[16-24]
### Suggested fix approach
1. Add `test.use({locale: 'en-US'});` near the top of each new spec file that compares against English strings.
2. Prefer locale-independent assertions:
- Instead of parsing with `new Date(text).getTime()`, assert `text` is non-empty and does not contain `NaN`.
- For the revision label, assert it matches a pattern like `/\d+/` (or read the slider value and only assert the label contains the revision number).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Numeric padId collision risk ✓ Resolved 🐞 Bug ☼ Reliability
Description
timeslider_export_links.spec.ts generates a numeric padId with only 1000 possible random suffixes,
which can collide across Playwright’s parallel workers and multiple browser projects, causing tests
to share the same pad and interfere with each other. This is avoidable flakiness given other helpers
already use UUID-backed pad IDs for uniqueness.
Code

src/tests/frontend-new/specs/timeslider_export_links.spec.ts[R52-57]

+    // A numeric pad id is the specific case the legacy test guarded — the
+    // href rewriter must not confuse it with the revision segment.
+    const padId = String(735773577357 + Math.floor(Math.random() * 1000));
+    await suppressDeletionTokenModal(page);
+    await goToPad(page, padId); // navigates and waits for the editor to be ready
+    await clearPadContent(page);
Evidence
The test’s padId generator has a small keyspace, and the repository Playwright config explicitly
enables parallel execution across workers/projects, making probabilistic collisions possible; the
existing pad helper demonstrates the preferred unique-id approach.

src/tests/frontend-new/specs/timeslider_export_links.spec.ts[51-56]
src/playwright.config.ts[28-67]
src/tests/frontend-new/helper/padHelper.ts[135-169]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The export-links test uses `735773577357 + Math.floor(Math.random() * 1000)` for its numeric pad id, giving only 1000 possible ids. Under Playwright parallelism (multiple workers + multiple browser projects), collisions are possible and can make test runs interfere by operating on the same pad.
## Issue Context
- Playwright is configured to run fully parallel with multiple workers and at least two projects (chromium + firefox).
- Other helpers already generate essentially-collision-free pad ids with `randomUUID()`.
## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_export_links.spec.ts[52-57]
### Suggested fix approach
Generate a numeric id with much higher entropy, e.g.:
- `const padId = String(Date.now()) + String(Math.floor(Math.random() * 1_000_000));`
- Or derive digits from a UUID (strip non-digits / hash to integer) while keeping the “numeric pad id” property.
(Keep the pad id numeric to preserve the original regression guard.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Port timeslider legacy Mocha specs to Playwright and remove orphaned suite
🧪 Tests 🕐 20-40 Minutes

Grey Divider

Walkthroughs

Description
• Add Playwright coverage for in-pad history banner, export links, and deep-link entry
• Retarget timeslider assertions from embedded iframe to real outer pad history UI
• Delete legacy Mocha timeslider specs that were not executed by CI
Diagram
graph TD
  A["Playwright runner (CI)"] --> B["New timeslider specs"] --> C["Pad editor UI"] --> D["History mode (outer UI)"]
  D --> E["Revision banner asserts"]
  D --> F["Export link asserts"]
  D --> G["Deep-link hash asserts"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Re-enable legacy Mocha frontend specs in CI
  • ➕ Lower initial porting effort (keep existing tests)
  • ➕ Preserves current assertions with minimal churn
  • ➖ Keeps two E2E stacks and helpers alive (higher long-term maintenance)
  • ➖ Legacy suite was already drifting and not aligned with current CI direction
  • ➖ Does not naturally encourage writing new coverage in the preferred framework
2. Keep iframe-focused Playwright tests and add separate outer-UI tests
  • ➕ Maintains coverage for embedded timeslider internals while adding in-pad coverage
  • ➕ Can isolate failures to inner vs outer UI
  • ➖ More total tests to maintain (duplicate revision-generation/setup)
  • ➖ Risk of asserting implementation details of the embedded iframe rather than user-facing behavior
3. Introduce shared history-mode helper utilities
  • ➕ Reduces duplication (enterHistoryMode, goToRevision, polling patterns)
  • ➕ Makes future timeslider tests cheaper to add and more consistent
  • ➖ Adds abstraction overhead now; may be premature if spec count stays small
  • ➖ Requires careful API design to avoid hiding necessary timing/await details

Recommendation: The PR’s approach (porting to Playwright and explicitly targeting the real in-pad/outer history UI) is the best long-term option: it closes the CI gap with tests that match user interaction surfaces and consolidates around the actively-run test framework. Consider a follow-up to factor common history-mode helpers once more specs accumulate.

Grey Divider

File Changes

Tests (3)
timeslider_deeplink.spec.ts Add Playwright coverage for timeslider deep-link entry +50/-0

Add Playwright coverage for timeslider deep-link entry

• Adds serial Playwright tests that navigate to /p/<pad>#rev/0 and legacy #0 to ensure the pad boots into history mode at the requested revision. Asserts history UI visibility and slider value to prevent regressions in hash bootstrap behavior.

src/tests/frontend-new/specs/timeslider_deeplink.spec.ts


timeslider_export_links.spec.ts Add Playwright tests for export href rewriting across revisions +86/-0

Add Playwright tests for export href rewriting across revisions

• Adds a serial Playwright spec that enters history mode and verifies outer export links include /p/<pad>/<rev>/export/<type>, specifically covering numeric pad IDs. Scrubs to revision 0 and asserts hrefs update accordingly.

src/tests/frontend-new/specs/timeslider_export_links.spec.ts


timeslider_revision_labels.spec.ts Add Playwright tests for outer history banner revision/date labels +77/-0

Add Playwright tests for outer history banner revision/date labels

• Adds a serial Playwright spec that generates multiple revisions, enters history mode, and validates the outer banner shows the correct Version label plus parseable date and timer. Scrubs to revision 0 and asserts labels/dates update and remain non-NaN.

src/tests/frontend-new/specs/timeslider_revision_labels.spec.ts


Grey Divider

Qodo Logo

JohnMcLear and others added 2 commits June 12, 2026 11:54
Assert the canonical #rev/0 URL and the slider landing on revision 0 rather
than the #history-banner-rev label text. The banner label is populated via a
MutationObserver bridge that races the iframe load on the bootstrap path in
Firefox (it is already covered for the normal button-entry flow by
timeslider_revision_labels.spec.ts); the URL + slider value are the
deterministic signals that the deep link entered history at the right revision.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Pin locale to en-US in the revision-labels spec so the localized 'Version N'
  label and 'Saved <Month> <day>, <year>' date assertions are deterministic and
  the date stays Date-parseable, instead of depending on the runner's locale.
- Use a high-entropy numeric pad id (timestamp + random) in the export-links
  spec so reruns against a persistent DB can't collide on the same pad.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JohnMcLear

Copy link
Copy Markdown
Member Author

Both addressed in 24f4d6d:

1. Locale-fragile banner assertions — Verified. The Version N label and Saved <Month> <day>, <year> date are localized (timeslider.version / timeslider.saved), so they're only deterministic under English. Pinned the spec to en-US via test.use({locale: 'en-US'}), which keeps the exact assertions valid and the date Date-parseable regardless of the runner's default locale.

2. Numeric padId collision risk — Verified. The base + random(1000) range could collide across reruns against a persistent DB. Switched to a high-entropy numeric id (${Date.now()}${random(1000)}), still numeric so it exercises the numeric-padId href-rewrite path.

Re-verified both specs on Chromium and Firefox.

@JohnMcLear JohnMcLear merged commit 73a9b88 into ether:develop Jun 12, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant