Skip to content
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.

Sign up for GitHub

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

feat: switch to the Firefox Stable equivalent by default #6926

Merged
merged 1 commit into from
Jun 7, 2021
Merged

feat: switch to the Firefox Stable equivalent by default #6926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/infra.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ jobs:
- uses: actions/setup-node@v2
with:
node-version: 12
- uses: microsoft/playwright-github-action@v1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR: We use microsoft/playwright-github-action in quite a few other places in this repository. Can we fully remove it from the repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yess good point

- run: npm ci
- run: npm run build
- run: node lib/cli/cli install-deps chromium firefox webkit
- run: npm run lint
- name: Verify clean tree
run: |
Expand All @@ -39,8 +39,8 @@ jobs:
- uses: actions/setup-node@v2
with:
node-version: 12
- uses: microsoft/playwright-github-action@v1
- run: npm ci
- run: npm run build
- run: node lib/cli/cli install-deps chromium
- run: node utils/build/update_canary_version.js --today-date
- run: utils/build/build-playwright-driver.sh
30 changes: 15 additions & 15 deletions .github/workflows/tests_secondary.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,8 @@ jobs:
name: chrome-stable-mac-test-results
path: test-results

firefox_stable_linux:
name: "Firefox Stable (Linux)"
firefox_beta_linux:
name: "Firefox Beta (Linux)"
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v2
Expand All @@ -270,20 +270,20 @@ jobs:
PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: 1
- run: npm run build
- run: node lib/cli/cli install-deps firefox
- run: node lib/cli/cli install firefox-stable chromium
- run: node lib/cli/cli install firefox chromium
- run: xvfb-run --auto-servernum --server-args="-screen 0 1280x960x24" -- npm run ftest
env:
PWTEST_CHANNEL: firefox-stable
PWTEST_CHANNEL: firefox-beta
- run: ./utils/upload_flakiness_dashboard.sh ./test-results/report.json
if: always()
- uses: actions/upload-artifact@v1
if: ${{ always() }}
with:
name: firefox-stable-linux-test-results
name: firefox-beta-linux-test-results
path: test-results

firefox_stable_win:
name: "Firefox Stable (Win)"
firefox_beta_win:
name: "Firefox Beta (Win)"
runs-on: windows-latest
steps:
- uses: actions/checkout@v2
Expand All @@ -295,22 +295,22 @@ jobs:
PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: 1
- run: npm run build
- run: node lib/cli/cli install-deps chromium
- run: node lib/cli/cli install firefox-stable chromium
- run: node lib/cli/cli install firefox chromium
- run: npm run ftest
shell: bash
env:
PWTEST_CHANNEL: firefox-stable
PWTEST_CHANNEL: firefox-beta
- run: ./utils/upload_flakiness_dashboard.sh ./test-results/report.json
if: always()
shell: bash
- uses: actions/upload-artifact@v1
if: ${{ always() }}
with:
name: firefox-stable-win-test-results
name: firefox-beta-win-test-results
path: test-results

firefox_stable_mac:
name: "Firefox Stable (Mac)"
firefox_beta_mac:
name: "Firefox Beta (Mac)"
runs-on: macos-10.15
steps:
- uses: actions/checkout@v2
Expand All @@ -321,16 +321,16 @@ jobs:
env:
PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: 1
- run: npm run build
- run: node lib/cli/cli install firefox-stable chromium
- run: node lib/cli/cli install firefox chromium
- run: npm run ftest
env:
PWTEST_CHANNEL: firefox-stable
PWTEST_CHANNEL: firefox-beta
- run: ./utils/upload_flakiness_dashboard.sh ./test-results/report.json
if: always()
- uses: actions/upload-artifact@v1
if: ${{ always() }}
with:
name: firefox-stable-mac-test-results
name: firefox-beta-mac-test-results
path: test-results

edge_stable_mac:
Expand Down
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# 🎭 Playwright

[![npm version](https://img.shields.io/npm/v/playwright.svg?style=flat)](https://www.npmjs.com/package/playwright) [![Join Slack](https://img.shields.io/badge/join-slack-infomational)](https://aka.ms/playwright-slack) <!-- GEN:chromium-version-badge -->[![Chromium version](https://img.shields.io/badge/chromium-93.0.4530.0-blue.svg?logo=google-chrome)](https://www.chromium.org/Home)<!-- GEN:stop --> <!-- GEN:firefox-version-badge -->[![Firefox version](https://img.shields.io/badge/firefox-89.0b15-blue.svg?logo=mozilla-firefox)](https://www.mozilla.org/en-US/firefox/new/)<!-- GEN:stop --> <!-- GEN:webkit-version-badge -->[![WebKit version](https://img.shields.io/badge/webkit-14.2-blue.svg?logo=safari)](https://webkit.org/)<!-- GEN:stop -->
[![npm version](https://img.shields.io/npm/v/playwright.svg?style=flat)](https://www.npmjs.com/package/playwright) [![Join Slack](https://img.shields.io/badge/join-slack-infomational)](https://aka.ms/playwright-slack) <!-- GEN:chromium-version-badge -->[![Chromium version](https://img.shields.io/badge/chromium-93.0.4530.0-blue.svg?logo=google-chrome)](https://www.chromium.org/Home)<!-- GEN:stop --> <!-- GEN:firefox-version-badge -->[![Firefox version](https://img.shields.io/badge/firefox-89.0-blue.svg?logo=mozilla-firefox)](https://www.mozilla.org/en-US/firefox/new/)<!-- GEN:stop --> <!-- GEN:webkit-version-badge -->[![WebKit version](https://img.shields.io/badge/webkit-14.2-blue.svg?logo=safari)](https://webkit.org/)<!-- GEN:stop -->

## [Documentation](https://playwright.dev) | [API reference](https://playwright.dev/docs/api/class-playwright/)

Expand All @@ -10,7 +10,7 @@ Playwright is a Node.js library to automate [Chromium](https://www.chromium.org/
| :--- | :---: | :---: | :---: |
| Chromium <!-- GEN:chromium-version -->93.0.4530.0<!-- GEN:stop --> | :white_check_mark: | :white_check_mark: | :white_check_mark: |
| WebKit <!-- GEN:webkit-version -->14.2<!-- GEN:stop --> | :white_check_mark: | :white_check_mark: | :white_check_mark: |
| Firefox <!-- GEN:firefox-version -->89.0b15<!-- GEN:stop --> | :white_check_mark: | :white_check_mark: | :white_check_mark: |
| Firefox <!-- GEN:firefox-version -->89.0<!-- GEN:stop --> | :white_check_mark: | :white_check_mark: | :white_check_mark: |

Headless execution is supported for all the browsers on all platforms. Check out [system requirements](https://playwright.dev/docs/intro/#system-requirements) for details.

Expand Down
4 changes: 2 additions & 2 deletions browsers.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@
{
"name": "firefox",
"revision": "1268",
"installByDefault": true
"installByDefault": false
},
{
"name": "firefox-stable",
"revision": "1259",
"installByDefault": false
"installByDefault": true
},
{
"name": "webkit",
Expand Down
6 changes: 3 additions & 3 deletions packages/build_package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const PLAYWRIGHT_CORE_FILES = ['bin', 'lib', 'types', 'NOTICE', 'LICENSE', ];
const PACKAGES = {
'playwright': {
description: 'A high-level API to automate web browsers',
browsers: ['chromium', 'firefox', 'webkit', 'ffmpeg'],
browsers: ['chromium', 'firefox-stable', 'webkit', 'ffmpeg'],
// We copy README.md additionally for Playwright so that it looks nice on NPM.
files: [...PLAYWRIGHT_CORE_FILES, 'README.md'],
},
Expand All @@ -44,7 +44,7 @@ const PACKAGES = {
},
'playwright-test': {
description: 'Playwright Test Runner',
browsers: ['chromium', 'firefox', 'webkit', 'ffmpeg'],
browsers: ['chromium', 'firefox-stable', 'webkit', 'ffmpeg'],
files: PLAYWRIGHT_CORE_FILES,
name: '@playwright/test',
},
Expand All @@ -55,7 +55,7 @@ const PACKAGES = {
},
'playwright-firefox': {
description: 'A high-level API to automate Firefox',
browsers: ['firefox'],
browsers: ['firefox-stable'],
files: PLAYWRIGHT_CORE_FILES,
},
'playwright-chromium': {
Expand Down
12 changes: 7 additions & 5 deletions src/server/browserType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,22 @@ import { CallMetadata, SdkObject } from './instrumentation';
const ARTIFACTS_FOLDER = path.join(os.tmpdir(), 'playwright-artifacts-');

export abstract class BrowserType extends SdkObject {
private _name: registry.BrowserName;
private _name: 'chromium'|'firefox'|'webkit';
private _binaryName: registry.BrowserName;
readonly _registry: registry.Registry;
readonly _playwrightOptions: PlaywrightOptions;

constructor(browserName: registry.BrowserName, playwrightOptions: PlaywrightOptions) {
constructor(name: 'chromium'|'firefox'|'webkit', binaryName: registry.BrowserName, playwrightOptions: PlaywrightOptions) {
super(playwrightOptions.rootSdkObject, 'browser-type');
this.attribution.browserType = this;
this._playwrightOptions = playwrightOptions;
this._name = browserName;
this._name = name;
this._binaryName = binaryName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not a huge fan of _binaryName. Maybe:

  • _name -> _baseBrowserName
  • _binaryName -> _browserName

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word "browser" is very overloaded, so I'd prefer to have distinct names for distinct entities.

To elaborate: both //src/utils/registry.ts and playwright servers //src/server/browserType.ts use word "browser" here and there, yet these are two very different things:

  • in playwright servers, "browser" means the browser interface we use to communicate. So we have "chromium" for CDP, "webkit" for WebInspector Remote Debugging Protocol, and "firefox" for juggler. These all are "browsers".
  • in registry, "browser" means an entity that we download from our CDN. Originally we were only downloading browsers, but we now have FFMPEG and we are considering moving WinLDD there as well.

If we were to start over, we'd establish some other terminology for the registry that doesn't include name "browser" - "binary" seems to be fine since it's distinct enough and is not overused around the codebase.

this._registry = playwrightOptions.registry;
}

executablePath(channel?: string): string {
return this._registry.executablePath(this._name) || '';
return this._registry.executablePath(this._binaryName) || '';
}

name(): string {
Expand Down Expand Up @@ -172,7 +174,7 @@ export abstract class BrowserType extends SdkObject {

// Only validate dependencies for downloadable browsers.
if (!executablePath && !options.channel)
await validateHostRequirements(this._registry, this._name);
await validateHostRequirements(this._registry, this._binaryName);
else if (!executablePath && options.channel && this._registry.isSupportedBrowser(options.channel))
await validateHostRequirements(this._registry, options.channel as registry.BrowserName);

Expand Down
2 changes: 1 addition & 1 deletion src/server/chromium/chromium.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class Chromium extends BrowserType {
private _devtools: CRDevTools | undefined;

constructor(playwrightOptions: PlaywrightOptions) {
super('chromium', playwrightOptions);
super('chromium', 'chromium', playwrightOptions);

if (debugMode())
this._devtools = this._createDevTools();
Expand Down
6 changes: 3 additions & 3 deletions src/server/firefox/firefox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ import * as types from '../types';

export class Firefox extends BrowserType {
constructor(playwrightOptions: PlaywrightOptions) {
super('firefox', playwrightOptions);
super('firefox', 'firefox-stable', playwrightOptions);
}

executablePath(channel?: string): string {
if (channel) {
let executablePath = undefined;
if ((channel as any) === 'firefox-stable')
executablePath = this._registry.executablePath('firefox-stable');
if ((channel as any) === 'firefox-beta')
executablePath = this._registry.executablePath('firefox');
assert(executablePath, `unsupported firefox channel "${channel}"`);
assert(fs.existsSync(executablePath), `"${channel}" channel is not installed. Try running 'npx playwright install ${channel}'`);
return executablePath;
Expand Down
2 changes: 1 addition & 1 deletion src/server/webkit/webkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { assert } from '../../utils/utils';

export class WebKit extends BrowserType {
constructor(playwrightOptions: PlaywrightOptions) {
super('webkit', playwrightOptions);
super('webkit', 'webkit', playwrightOptions);
}

executablePath(channel?: string): string {
Expand Down