Fix Chromium version - #2
Conversation
9a67adc to
c481a05
Compare
| let driver; | ||
|
|
||
| before(async function() { | ||
| const buildId = fs.readFileSync('.browser', 'utf8').trim(); |
There was a problem hiding this comment.
Should we make this optional — by default it would download the latest, so that we don't have to keep the templates updated as newer Chromes release, but have the option available who wants specific version to reproduce an issue?
I'm thinking of moving version out of .browser and into here with a getSpecificChromeVersion(version: string): string function within the template, and having that line commented by default, or something like that? WDYT?
There was a problem hiding this comment.
done. Added respect for env variable BROWSER_VERSION and documented it.
| @@ -17,17 +17,40 @@ | |||
| const { Builder, By, until } = require('selenium-webdriver'); | |||
There was a problem hiding this comment.
This is really cool, and thanks for the corrections! We'll need to update the other non-js templates as well.
d825ec6 to
3e02162
Compare
|
@alexnj I assume I addressed your issues and merging this PR to unblock myself. I can address any other concerns in a follow-up PR. |
alexnj
left a comment
alexnj
left a comment
There was a problem hiding this comment.
Added some more feedback.
| run: npm test | ||
|
|
||
| - name: Dump log | ||
| - name: Dump latest ChromeDriver log |
There was a problem hiding this comment.
Why do we need to keep non-latest logs?
There was a problem hiding this comment.
This will need to be reverted, as failed chrome logging picks up the last known good log -- which is confusing.
| @@ -1,28 +1,99 @@ | |||
| # Template for macOS, Selenium, MochaJS | |||
| # Template for Selenium, MochaJS | |||
There was a problem hiding this comment.
This template uses the latest macOS.
| BROWSER_VERSION=144.0.7557.0 npm test | ||
| ``` | ||
|
|
||
| If `BROWSER_VERSION` is not provided, the script automatically resolves and downloads |
There was a problem hiding this comment.
Probably need to mention where one needs to provide BROWSER_VERSION, preferably in a way that works across all templates. Perhaps this can be a variable in the test script itself.
|
|
||
| Verbose logging for ChromeDriver is enabled by default. Logs are captured and saved | ||
| to individual files within the **`logs/`** directory, with each filename timestamped | ||
| (e.g., `logs/chromedriver-2025-12-02T10:00:00.000Z.log`). This ensures that logs are |
There was a problem hiding this comment.
Wouldn't log of each run be preserved by the CI file system itself, for each run?
| before(async function () { | ||
| // The chrome and chromedriver installation can take some time. Give 5 | ||
| // minutes to install everything. | ||
| this.timeout(5 * 60 * 1000); |
There was a problem hiding this comment.
Is this specific to non-standard/non-Canary builds?
| detectBrowserPlatform, | ||
| ChromeReleaseChannel, | ||
| } = require('@puppeteer/browsers'); | ||
| const winston = require('winston'); |
There was a problem hiding this comment.
Do we need this dependency?
| * environment variable like `BROWSER_VERSION=142.0.7444.175` | ||
| */ | ||
| const BROWSER_VERSION = | ||
| process.env['BROWSER_VERSION'] ?? |
There was a problem hiding this comment.
Hmm I was hoping we could keep the test script rather simple, so it's not confusing for an end user to know what to modify. Perhaps we should move these boilerplate to a separate file and just include, so that the test script itself is short (and sweet?)
2 participants
Instead of relying on local setup, use the consistent browser version. This helps for repro issues in the specific chrome versions.