New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test configuration options for running tests against multiple products #5964
Conversation
The extra Launcher options and skipping conditions enable unit tests to be run more easily by third-parties, e.g. browser vendors that are interested in Puppeteer support. Extra Launcher options were previously removed as part of switching away from the custom test harness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited about the many unskipped tests! LGTM, but @jackfranklin PTAL as well.
@@ -415,9 +415,6 @@ function extractTar(tarPath: string, folderPath: string): Promise<unknown> { | |||
tarStream.on('error', reject); | |||
tarStream.on('finish', fulfill); | |||
const readStream = fs.createReadStream(tarPath); | |||
readStream.on('data', () => { | |||
process.stdout.write('\rExtracting...'); | |||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been meaning to get rid of this for so long, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So exciting to see many more tests not skipped!
This is failing on CI on macOS with the following:
@jackfranklin does Travis somehow get puppeteer/test/navigation.spec.js Lines 151 to 159 in 6cfe142
To resolve this, instead of the current check, we could just check for the presence of both the substrings |
test/navigation.spec.js
Outdated
.goto(httpsServer.PREFIX + '/redirect/1.html') | ||
.catch((error_) => (error = error_)); | ||
if (isChrome) | ||
expect(error.message).toContain('net::ERR_CERT_AUTHORITY_INVALID'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjzffr I think you need to change the assertion here back to what it was - the EXPECTED_SSL_CERT_MESSAGE
constant. See the comment further up this file about some slight oddness with Chrome + Mac versions that Travis hits vs what we get locally. Did you need to change this to get the test passing for you? There may well be a better way of dealing of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change may just have been the result of a bad merge on my end.
bda9164
to
50b5d13
Compare
@mjzffr this looks good except you left the |
50b5d13
to
42897bc
Compare
I think a webhook didn't fire here because the build is green on Travis but GH doesn't know about it...going to prod it to rebuild. |
…ple products (#5964) * chore: remove "Extracting..." log message Fixes #5741. * test: support extra Launcher options and skips The extra Launcher options and skipping conditions enable unit tests to be run more easily by third-parties, e.g. browser vendors that are interested in Puppeteer support. Extra Launcher options were previously removed as part of switching away from the custom test harness. * test: enable more tests for Firefox
This is in support of Mozilla's internal sync to Puppeteer 3.1 at https://bugzilla.mozilla.org/show_bug.cgi?id=1632710
Notably add
itChromeOnly
,itRegularInstallOnly
Also unskip more Firefox tests in Puppeteer CI.