feat: switch to the Firefox Stable equivalent by default #6926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aslushnikov
force-pushed
the
switch-to-ff-stable-by-default
branch
2 times, most recently
from
f96c902
to
f0b6782
Compare
mxschmitt
approved these changes
Jun 7, 2021
| @@ -19,9 +19,9 @@ jobs: | |||
| - uses: actions/setup-node@v2 | |||
| with: | |||
| node-version: 12 | |||
| - uses: microsoft/playwright-github-action@v1 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yess good point
| super(playwrightOptions.rootSdkObject, 'browser-type'); | ||
| this.attribution.browserType = this; | ||
| this._playwrightOptions = playwrightOptions; | ||
| this._name = browserName; | ||
| this._name = name; | ||
| this._binaryName = binaryName; |
There was a problem hiding this comment.
nit: I'm not a huge fan of _binaryName. Maybe:
_name->_baseBrowserName_binaryName->_browserName
There was a problem hiding this comment.
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 patch: - starts downloading Firefox Stable equivalent by default - starts running Firefox-Stable on our smoke tests (tests-1) - starts running Firefox-Beta on our CQ1 tests (tests-2) Note: there's a little confusion right now with browser names: - `firefox-stable` - firefox-stable equivalent - `firefox`- firefox-beta equivalent I'll rename `firefox` to `firefox-beta` in a follow-up. Fixes microsoft#6817
aslushnikov
force-pushed
the
switch-to-ff-stable-by-default
branch
from
6343ddb
to
1e6a200
Compare
aslushnikov
added a commit
to aslushnikov/playwright
that referenced
this pull request
Jun 7, 2021
) This patch: - starts downloading Firefox Stable equivalent by default - starts running Firefox-Stable on our smoke tests (tests-1) - starts running Firefox-Beta on our CQ1 tests (tests-2) Note: there's a little confusion right now with browser names: - `firefox-stable` - firefox-stable equivalent - `firefox`- firefox-beta equivalent I'll rename `firefox` to `firefox-beta` in a follow-up. Fixes microsoft#6817
aslushnikov
added a commit
to aslushnikov/playwright
that referenced
this pull request
Jun 7, 2021
…rosoft#6926)" This reverts commit a25b116. In a discussion with Dmitry Gozman we decided to revert this and instead proceed with the following approach: - rename `//browser_patches/firefox` to `//browser_patches/firefox-beta` - rename `//browser_patches/firefox-stable` folder to `//browser_patches/firefox` In all of the folders, we will keep the `BUILD_NUMBER` original so that it doesn't clash on the CDN.
aslushnikov
added a commit
to aslushnikov/playwright
that referenced
this pull request
Jun 7, 2021
…rosoft#6926)" This reverts commit a25b116. In a discussion with Dmitry Gozman we decided to revert this and instead proceed with the following approach: - rename `//browser_patches/firefox` to `//browser_patches/firefox-beta` - rename `//browser_patches/firefox-stable` folder to `//browser_patches/firefox` In all of the folders, we will keep the `BUILD_NUMBER` original so that it doesn't clash on the CDN.
aslushnikov
added a commit
that referenced
this pull request
Jun 7, 2021
…)" (#6947) This reverts commit a25b116. In a discussion with Dmitry Gozman we decided to revert this and instead proceed with the following approach: - rename `//browser_patches/firefox` to `//browser_patches/firefox-beta` - rename `//browser_patches/firefox-stable` folder to `//browser_patches/firefox` In all of the folders, we will keep the `BUILD_NUMBER` original so that it doesn't clash on the CDN.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch:
Note: there's a little confusion right now with browser names:
firefox-stable- firefox-stable equivalentfirefox- firefox-beta equivalentI'll rename
firefoxtofirefox-betain a follow-up.Fixes #6817