Skip to content

Commit

Permalink
build: switch to macos "large" to fix missing Firefox
Browse files Browse the repository at this point in the history
The Arm64 images have been catered to Google and its Chrome browser
only, omitting Firefox.

Ref actions/runner-images#9974
  • Loading branch information
Krinkle committed Dec 31, 2024
1 parent 95b5d00 commit 8c79448
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 9 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/CI.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ jobs:
node: 20.x

# Includes Firefox, Google Chrome, Safari
# https://github.com/actions/runner-images/blob/macos-15/20241217.493/images/macos/macos-15-Readme.md
# https://github.com/actions/runner-images/blob/macos-13/20241216.479/images/macos/macos-13-Readme.md
- name: "macOS: Node 20"
os: macos-15
os: macos-13
node: 20.x

name: ${{ matrix.name }}
Expand Down
28 changes: 21 additions & 7 deletions src/browsers.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,16 @@ class LocalBrowser {
let exe;
for (const candidate of candidates) {
if (candidate !== null) {
logger.debug('browser_exe_check', candidate);
// Optimization: Use fs.existsSync. It is on par with accessSync and statSync,
// and beats concurrent fs/promises.access(cb) via Promise.all().
// Starting the promise chain alone takes the same time as a loop with
// 5x existsSync(), not even counting the await and boilerplate to manage it all.
if (fs.existsSync(candidate)) {
logger.debug('browser_exe_found');
logger.debug('browser_exe_found', candidate);
exe = candidate;
break;
} else {
logger.debug('browser_exe_check', candidate);
}
}
}
Expand Down Expand Up @@ -162,7 +163,7 @@ class LocalBrowser {
}

class FirefoxBrowser {
* getCandidates () {
static * #getCandidates () {
if (process.env.FIREFOX_BIN) yield process.env.FIREFOX_BIN;

// Find /usr/bin/firefox on platforms like linux (including WSL), freebsd, openbsd.
Expand All @@ -183,7 +184,7 @@ class FirefoxBrowser {
// if (LocalBrowser.isWsl()) { }
}

static createPrefsJs (prefs) {
static #createPrefsJs (prefs) {
let js = '';
for (const key in prefs) {
js += 'user_pref("' + key + '", ' + JSON.stringify(prefs[key]) + ');\n';
Expand All @@ -203,7 +204,7 @@ class FirefoxBrowser {
// https://github.com/airtap/the-last-browser-launcher/blob/v0.1.1/lib/launch/index.js
// https://github.com/karma-runner/karma-firefox-launcher/blob/v2.1.3/index.js
logger.debug('firefox_prefs_create', 'Creating temporary prefs.js file');
fs.writeFileSync(path.join(profileDir, 'prefs.js'), FirefoxBrowser.createPrefsJs({
fs.writeFileSync(path.join(profileDir, 'prefs.js'), FirefoxBrowser.#createPrefsJs({
'app.update.disabledForTesting': true, // Disable auto-updates
'browser.sessionstore.resume_from_crash': false,
'browser.shell.checkDefaultBrowser': false,
Expand Down Expand Up @@ -234,9 +235,22 @@ class FirefoxBrowser {
}));

try {
await LocalBrowser.spawn(this.getCandidates(), args, signal, logger);
await LocalBrowser.spawn(FirefoxBrowser.#getCandidates(), args, signal, logger);
} finally {
fs.rmSync(profileDir, { recursive: true, force: true });
// On Windows, when spawn() returns after firefox.exe has stopped, it sometimes can't delete
// some temporary files yet as they're somehow still in use (EBUSY). Perhaps a race condition,
// or an lagged background process?
// > BUSY: resource busy or locked,
// > unlink 'C:\Users\RUNNER~1\AppData\Local\Temp\qtap_EZ4IoO\bounce-tracking-protection.sqlite'
// Enable `maxRetries` to cover the common case, and beyond that try-catch
// as it is not critical for test completion.
// TODO: Can we abstract this so that it happens automatically for any directory
// obtained via LocalBrowser.makeTempDir?
try {
fs.rmSync(profileDir, { recursive: true, force: true, maxRetries: 2 });
} catch (e) {
logger.warning('firefox_cleanup_fail', e);
}
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/qtap.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ async function run (browserNames, files, options) {
// exits naturally by itself.
// TODO: Consider just calling process.exit after this await.
// Is that faster and safe? What if any important clean up would we miss?
// 1. Removing of temp directories is generally done in browser "launch" functions
// after the child process has properly ended (and has to, as otherwise the
// files are likely still locked and/or may end up re-created). If we were to
// exit earlier, we may leave temp directories behind. This is fine when running
// in an ephemeral environment (e.g. CI), but not great for local dev.
//
await Promise.allSettled(browserLaunches);
// Await again, so that any error gets thrown accordingly,
// we don't do this directly because we first want to wait for all tests
Expand Down

0 comments on commit 8c79448

Please sign in to comment.