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 02bb25e
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 3 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-14/20241216.504/images/macos/macos-14-Readme.md
- name: "macOS: Node 20"
os: macos-15
os: macos-14-large
node: 20.x

name: ${{ matrix.name }}
Expand Down
15 changes: 14 additions & 1 deletion src/browsers.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,20 @@ class FirefoxBrowser {
try {
await LocalBrowser.spawn(this.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 02bb25e

Please sign in to comment.