Skip to content

feat: add local Safari testing via safaridriver#5131

Draft
marcoscaceres wants to merge 28 commits into
mainfrom
chore/safari-launcher
Draft

feat: add local Safari testing via safaridriver#5131
marcoscaceres wants to merge 28 commits into
mainfrom
chore/safari-launcher

Conversation

@marcoscaceres
Copy link
Copy Markdown
Contributor

@marcoscaceres marcoscaceres commented Mar 27, 2026

Enables pnpm start --browser Safari for local testing and adds Safari to CI.

Problem

karma-safari-launcher@1.0.0 (the published package) uses a redirect.html hack that modern Safari treats as a file download rather than opening it. @onslap/karma-safari-launcher (the replacement) depends on wd@1.x which requires a native build that doesn't compile on Node 24+.

Solution

A minimal custom karma launcher (tests/karma.safari.cjs) that talks directly to safaridriver over W3C WebDriver HTTP protocol using Node's built-in http module. No new npm dependencies.

Also fixes Safari-specific issues discovered during testing:

  • postMessage from srcdoc iframes: skip when parent.location.origin throws SecurityError (Safari opaque-origin restriction) instead of falling back to targetOrigin: "*"
  • ev.data guard in message handler to prevent crashes from non-object payloads
  • Error normalization in SpecHelper.js for Safari (undefined rejection reasons crash jasmine-core)
  • Polling fallback in waitReady for Safari where ev.source !== iframe.contentWindow
  • textContent instead of innerText for highlight (Safari returns empty for closed <details>)
  • parseInfoString null guard for Safari's marked tokenizer

Setup (one-time per machine)

sudo safaridriver --enable

CI

Safari runs on macos-latest in both PR and push workflows. macOS runners cost more than Linux but catching Safari-specific regressions is worth it.

Closes #2894

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a custom Karma launcher to enable running the project’s unit tests locally in Safari via safaridriver, replacing the deprecated/broken third-party Safari launcher dependency.

Changes:

  • Introduces tests/karma.safari.cjs, a minimal W3C WebDriver client/launcher that spawns safaridriver and navigates to the Karma URL.
  • Wires the custom launcher into the shared Karma config (tests/karma.conf.base.cjs).
  • Removes karma-safari-launcher from dependencies/lockfile.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File Description
tests/karma.safari.cjs New custom Safari launcher using safaridriver + Node http.
tests/karma.conf.base.cjs Replaces karma-safari-launcher plugin with the local Safari launcher.
package.json Drops karma-safari-launcher dependency.
pnpm-lock.yaml Removes karma-safari-launcher entries.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/karma.safari.cjs Outdated
Comment thread tests/karma.safari.cjs Outdated
Comment thread tests/karma.safari.cjs
Comment thread tests/karma.safari.cjs
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/karma.safari.cjs Outdated
Comment thread tests/karma.safari.cjs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/karma.safari.cjs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/karma.safari.cjs Outdated
Comment thread src/core/markdown.js
@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot i think we should enable this by default to run alongside Chrome and Firefox, even if it’s more expensive to run on macOS in CI. We care that we don’t regress on WebKit/Safari so it’s worth it.

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

@copilot i think we should enable this by default to run alongside Chrome and Firefox, even if it’s more expensive to run on macOS in CI. We care that we don’t regress on WebKit/Safari so it’s w...

Done in bd0debf — Safari is now enabled in CI on macos-latest runners alongside Chrome and Firefox:

  • pr.yml: extended the test-karma matrix to include Safari / macos-latest (with sudo safaridriver --enable as a conditional step before installing deps).
  • push.yml: added a new test-karma-safari job on macos-latest that runs both test:unit and test:integration with BROWSERS: Safari.

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot Can you check and fix ci?

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

@copilot Can you check and fix ci?

Fixed in a7d2784. The CI failure had two root causes on cold macOS runners:

  1. Fixed 500ms startup wait was too short — safaridriver wasn't listening yet, so POST /session got ECONNREFUSED immediately. Replaced with a waitForReady() helper that polls GET /status every 250ms for up to 15s, only proceeding once safaridriver responds.
  2. 10s POST /session timeout was too short — Safari itself takes time to launch on a cold runner. Increased to 30s for that call.

The waitForReady also properly races against the process error/exit events (with a settle-once guard), so early failures are still reported quickly with a clear message.

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot Couple of lint errors to fix. See ci.

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

@copilot Couple of lint errors to fix. See ci.

Fixed in dab0d02 — two prettier formatting errors in tests/karma.safari.cjs:

  1. Long reject(new Error(...)) call on line 108 → wrapped to multi-line
  2. Curried arrow const finish = fn => (...args) => { on lines 157–161 → reformatted to prettier's preferred style for curried arrows

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

Seeing a lot ci fails @copilot. Do your thing, my good every so helpful little digital marvel.

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

Seeing a lot ci fails @copilot. Do your thing, my good every so helpful little digital marvel.

Fixed in ad409ca — two root causes identified from the Safari CI log:

  1. src/core/pubsubhub.js: window.parent.location.origin throws SecurityError in Safari when srcdoc iframes are treated as opaque-origin. Wrapped in try/catch with "*" as a safe fallback (these messages are test-only, contain no sensitive data).

  2. tests/unit/SpecHelper.js: the waitReady message handler used ev.source.document, which also throws SecurityError cross-origin in Safari, silently preventing the "end-all" message from being handled and leaving tests hanging. Replaced with ev.source !== iframe.contentWindow (safe cross-origin Window identity comparison). Also changed the timeout from throw inside setTimeout (which Safari converts to an unhandled rejection with undefined, crashing jasmine-core) to a proper reject() inside the Promise.

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot can you review this again and fix any issues you find and any resolve any unresolved comments?

Copilot AI and others added 13 commits April 28, 2026 12:36
Agent-Logs-Url: https://github.com/speced/respec/sessions/8e1b50fe-6001-4593-9c5d-d20618791bcb

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
…I reliability

Agent-Logs-Url: https://github.com/speced/respec/sessions/75aecc21-a5d1-4831-a107-5a4f42a262a9

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Agent-Logs-Url: https://github.com/speced/respec/sessions/f825ab10-4ee2-427b-9176-637faabea744

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
…tReady

Agent-Logs-Url: https://github.com/speced/respec/sessions/8f5dd784-fefd-442c-970b-ce6991b8fd8c

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
…down

Agent-Logs-Url: https://github.com/speced/respec/sessions/4c275e23-6d86-4c63-9268-4b625420bb32

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
In Safari, postMessage from opaque-origin srcdoc iframes may not be
received (ev.source !== iframe.contentWindow). Add a 100ms polling
loop that checks doc.respec.ready directly, racing the existing
postMessage listener. Also:
- Fix ev.source check to accept null/undefined (loose !=)
- Wrap window.parent.postMessage in pubsubhub.js try/catch so
  postMessage failures don't propagate through pub() and disrupt runAll()
- Suppress Promise.reject(err) unhandled rejection in iframe template
  so Safari doesn't cross-frame propagate it as undefined to jasmine

Agent-Logs-Url: https://github.com/speced/respec/sessions/e996459b-6f02-4d6b-9ade-d9003140e5a4

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Agent-Logs-Url: https://github.com/speced/respec/sessions/65cc375e-3e6f-43b8-9cf6-3e663ae1544c

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
…ch errors

Agent-Logs-Url: https://github.com/speced/respec/sessions/da22e939-2ed4-4370-9fc5-2450af0e5876

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Instead of falling back to targetOrigin '*' (which leaks
respecConfig data to any embedding parent), return early when
parent.location.origin throws SecurityError. The polling
fallback in SpecHelper.js handles test completion in Safari.

Also guard ev.data access in msgHandler to prevent crashes
from non-object message payloads.
Throw early if env var is set to a non-numeric value instead of
silently passing NaN as the port argument to safaridriver.
…igin iframes; add port range validation

Agent-Logs-Url: https://github.com/speced/respec/sessions/7457d32d-c547-4cd4-94fe-bab03a6343da

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
…e for heading (#5207)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
)

Co-authored-by: Tim van der Lippe <Tim.Lippe@logius.nl>
Co-authored-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@marcoscaceres marcoscaceres force-pushed the chore/safari-launcher branch from f6388e8 to d5a21e6 Compare April 28, 2026 02:37
marcoscaceres and others added 3 commits April 30, 2026 06:21
Root cause: Safari ES module loader cannot resolve absolute paths
from about:srcdoc documents (no origin to resolve against). Chrome
and Firefox inherit the parent origin, but Safari does not.

Fix: pass window.location.origin into the iframe script and build
fully-qualified import URLs. Also add a base tag and unhandled
rejection handler as defense in depth.
@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot can you fix the CI issues?

…; (Safari)

Agent-Logs-Url: https://github.com/speced/respec/sessions/2c2ebc73-4a2f-4b6a-ace4-2dcfba2f7105

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

@copilot can you fix the CI issues?

Fixed in 9a55b96. The 4 Safari-specific test failures had the same root cause: Safari returns '' for innerText on elements inside closed <details> elements (no layout is computed for hidden content).

  • data-tests-spec.js (3 failures): File/emoji anchor elements live inside <details class="respec-tests-details"> which is closed by default — changed to textContent.
  • issues-notes-spec.js (1 failure at line 447): The processed .note div — changed div.innerText to div.textContent.

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot still seeing issues?

The same Safari SecurityError bug that was fixed in tests/unit/SpecHelper.js
was also present in tests/spec/SpecHelper.js. The msgHandler in makeRSDoc:
- accessed ev.source.document which throws SecurityError in Safari for
  opaque-origin srcdoc iframes
- accessed ev.data.topic without guarding against non-object payloads

Changes:
- Add settle() guard for single resolution
- Replace unsafe ev.source.document check with try/catch
- Guard ev.data with typeof check before reading .topic
- Add 100ms polling fallback (same as unit SpecHelper) so tests complete
  even when postMessage isn't delivered reliably on Safari
- Clear timeout and listeners on early resolution via doc.respec

Agent-Logs-Url: https://github.com/speced/respec/sessions/d24e60b2-e396-49ad-a44b-7ccc2fc5e9da

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

@copilot still seeing issues?

Fixed in 79aec16. The root cause was that tests/spec/SpecHelper.js had the same Safari postMessage bug that was previously fixed in tests/unit/SpecHelper.jsev.source.document throws SecurityError in Safari for opaque-origin srcdoc iframes, causing the "end-all" message handler to fail silently and tests to time out.

The fix applies the same pattern: settle() guard for single resolution, try/catch around the ev.source.document check, ev.data type guard, and a 100ms polling fallback on doc.respec.ready.

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.

3 participants