test(windows): run the integration suite on Windows by freema · Pull Request #169 · mozilla/firefox-devtools-mcp

freema · 2026-08-23T19:29:32Z

Trimmed to the one commit left after the split: #172 (paths), #173 (dev tooling) and #174 (Firefox binary detection) carry the rest, and the tests/setup.ts cleanup now comes from #165.

What it does:

drops the Windows exclusion in vitest.config.ts and the standalone runner (scripts/run-integration-tests-windows.mjs, test:integration:win); the vitest fork hang from #33 no longer reproduces on Vitest 4
builds fixture URLs with pathToFileURL (fixtureUrl()), packs the extension fixture in-process instead of shelling out to zip, and retries temp-dir removal (removeDir())
runs ci.yml on windows-latest next to ubuntu-latest

Verified on Windows (7 integration files, 54 tests) and macOS (unit 608, integration 54).

f3tchcodes · 2026-08-24T10:28:38Z

FYI, there's some overlap with #165 which already covers Windows process cleanup in tests/setup.ts, and #164 covers the .gitattributes LF fix. Mentioning in case it's useful to avoid duplicate work while reviewing this PR.

juliandescottes

Thanks for the patch. I haven't fully tested, but it would be great to have more granular PRs. Could you split this ?

juliandescottes · 2026-08-24T12:29:53Z


 ## [Unreleased]

+### Added


This is too much for a changelog. Can you keep it short and focus on the impact for users?

juliandescottes · 2026-08-24T12:34:17Z

+ * On Windows geckodriver only searches the Program Files directories and
+ * HKEY_LOCAL_MACHINE, so it cannot see a per-user install (%LOCALAPPDATA%,
+ * registered under HKCU) — what the installer produces without admin rights.
+ * Finding the binary here and passing it as moz:firefoxOptions.binary fixes
+ * that. Windows-only: geckodriver's own lookup suffices elsewhere.


This sounds like something which should be fixed upstream in geckodriver rather than handled in the MCP with a workaround.

I think this is captured at https://bugzilla.mozilla.org/show_bug.cgi?id=1921933

(Which is a mentored bug, in case you're interested to do a geckodriver contribution :) )

juliandescottes · 2026-08-24T12:36:37Z

+- **Firefox discovery**: geckodriver only searches `%ProgramFiles%` and
+  `HKEY_LOCAL_MACHINE`, so it cannot find a per-user install
+  (`%LOCALAPPDATA%\Mozilla Firefox`). `src/firefox/binary.ts` resolves the binary
+  and passes it as `moz:firefoxOptions.binary`.
+- **Fixture URLs**: build them with `fixtureUrl()` from `tests/helpers/firefox.ts`.
+  Interpolating `file://${path}` yields `file://C:\...`, which never resolves
+  because the drive letter is parsed as the host.
+- **Process cleanup**: `tests/setup.ts` uses `taskkill` on Windows; pgrep/pkill
+  do not exist there.
+- **Deleting temp dirs**: use `removeDir()` from `tests/helpers/fs.ts`. Plain
+  `rmSync({ force: true })` intermittently throws `ENOTEMPTY` while handles close.
+- **Line endings**: `.gitattributes` checks out text files as LF. Without it
+  `core.autocrlf=true` makes `npm run format:check` fail on every file.
+- **Path comparisons**: compare against a root with `isWithinRoot()`
+  (`src/utils/save-output.ts`). Windows paths are case-insensitive, so a plain
+  `startsWith` rejects valid paths that differ only in case.
+- **`process.env` in tests**: the real environment is case-insensitive on Windows
+  (`process.env.SystemRoot` resolves the `SYSTEMROOT` key). Replacing it with a
+  plain object drops that, so set the variables a test needs explicitly.


This mostly repeats what the commit has been fixing. The documentation files are quite big already, I would skip that.

juliandescottes · 2026-08-24T12:37:18Z

+- vitest used to hang when forking integration tests that spawn Firefox on Windows (issue #33), so those tests were excluded there and a standalone runner (`scripts/run-integration-tests-windows.mjs`) stood in for them. The hang no longer reproduces on vitest 4, so the exclusion and the standalone runner were both removed and Windows runs the same tests as every other platform.
+- geckodriver only searches the Program Files directories and HKEY_LOCAL_MACHINE for Firefox, so it cannot see a per-user install (`%LOCALAPPDATA%\Mozilla Firefox`), which is what the installer produces without administrator rights. `src/firefox/binary.ts` locates the binary itself and passes it as `moz:firefoxOptions.binary`; use `--firefox-path` if an install still is not found.
+- `.gitattributes` checks out text files with LF everywhere. Without it, Git for Windows' `core.autocrlf=true` gives the working tree CRLF endings and `npm run format:check` fails on every file.


Repeats what the commit does, not worth keeping in the docs.

juliandescottes · 2026-08-24T12:39:52Z

+  // mkdirSync rather than shelling out to `mkdir -p`, which does not exist in
+  // the cmd.exe that npm runs scripts through on Windows.


I would remove this, it only make sense when looking at the diff.

juliandescottes · 2026-08-24T12:46:09Z

      firefoxOptions.enableBidi();

+      // True when no binary was found on Windows, so the failure can say why.
+      let binaryLookupFailed = false;


The variable name should make it clear this is windows only

juliandescottes · 2026-08-24T12:46:31Z

+  'Firefox could not be found on this system. geckodriver only searches the Program Files ' +
+  'directories and HKEY_LOCAL_MACHINE, so a Firefox installed for the current user only is ' +
+  'not detected automatically. Pass the full path to firefox.exe via --firefox-path ' +
+  '(for example --firefox-path "%LOCALAPPDATA%\\Mozilla Firefox\\firefox.exe"), or reinstall ' +
+  'Firefox for all users.';


I thought this binary.ts helper was meant to bypass the geckodriver limitation, but the message says that user installed Firefox are not handled?

I would go for a shorter message. eg

Unable to detect Firefox binary automatically, please provide the full path via --firefox-path

Also, why define this constant here if it's only used in core.ts?

juliandescottes · 2026-08-24T12:53:02Z

+*.bat text eol=crlf
+*.cmd text eol=crlf
+*.ps1 text eol=crlf


I don't see any file with those extensions in the repository, is this necessary? Otherwise this would be covered by #164 already?

The vitest fork hang (#33) no longer reproduces on Vitest 4, so drop the Windows exclusion and the standalone runner, fix fixture URLs and .xpi packing for Windows, and run CI on windows-latest too.

freema · 2026-08-26T14:39:02Z

Trimmed as discussed, this is now only the integration-suite commit on top of #165; the rest lives in #172, #173 and #174, and the docs/changelog additions went with it. Thanks for the split suggestion.

juliandescottes

Thanks, overall this looks good, but I don't think we should re-implement a zip helper here, there are many options to choose from.

juliandescottes · 2026-08-26T21:07:16Z

@@ -0,0 +1,123 @@
+/**


Is there a strong reason to re-implement a zip writer here? We already pull:

fflate

jszip

zip.js

fflate seems the most straightforward choice here. It's lightweight and has a sync API. Can you try to use it instead?

juliandescottes · 2026-08-26T21:12:34Z

@@ -100,7 +101,7 @@ describe('Console Tools', () => {
    afterEach(() => {
      vi.restoreAllMocks();
      if (existsSync(tempDir)) {


Nothing new from your PR, but in theory force: true ignores errors if the path does not exist, so those guards are most likely redundant. We could remove them in another PR.

juliandescottes requested changes Aug 24, 2026

View reviewed changes

juliandescottes reviewed Aug 24, 2026

View reviewed changes

This was referenced Aug 24, 2026

chore: enforce LF line endings with gitattributes #164

Merged

test: make process cleanup work on Windows #165

Merged

This was referenced Aug 25, 2026

fix(windows): handle file paths correctly on Windows #172

Merged

chore: make dev tooling cross-platform #173

Merged

fix(windows): auto-detect Firefox binary for per-user installs #174

Open

test(windows): run the integration suite on Windows

3bfc951

The vitest fork hang (#33) no longer reproduces on Vitest 4, so drop the Windows exclusion and the standalone runner, fix fixture URLs and .xpi packing for Windows, and run CI on windows-latest too.

freema force-pushed the fix/windows-compat branch from f1c0746 to 3bfc951 Compare August 26, 2026 14:13

freema changed the title ~~fix(windows): make the server and test suite work on Windows~~ test(windows): run the integration suite on Windows Aug 26, 2026

juliandescottes requested changes Aug 26, 2026

View reviewed changes

		// mkdirSync rather than shelling out to `mkdir -p`, which does not exist in
		// the cmd.exe that npm runs scripts through on Windows.

Uh oh!

Conversation

freema commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

f3tchcodes commented Aug 24, 2026

Uh oh!

juliandescottes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliandescottes Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliandescottes Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freema commented Aug 26, 2026

Uh oh!

juliandescottes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

freema commented Aug 23, 2026 •

edited

Loading

juliandescottes Aug 24, 2026 •

edited

Loading

juliandescottes Aug 24, 2026 •

edited

Loading