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.

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

fix: browser dependency validation #6227

Merged

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Apr 19, 2021

Refactoring the logic which was broken since #5820

This means:

  • channels? -> the channels which are in the browsers.json should be validated. The other browsers not.
  • executablePath given? no validation, the user knows what he is doing
  • no executable path given? validation, default behavior

Fixes #6226

@mxschmitt mxschmitt force-pushed the bugfix/host-dependencies-validation branch 2 times, most recently from 67005e3 to f0a30c4 Compare April 19, 2021 13:27
@aslushnikov
Copy link
Collaborator

channels? -> no validation, e.g. apt/yum should install all dependencies

Channels come from apt or from the npx playwright install command, e.g. npx playwright install webkit-technology-preview. So some of the channels do make sense to validate!

@mxschmitt mxschmitt force-pushed the bugfix/host-dependencies-validation branch 2 times, most recently from b328f21 to 8d77591 Compare April 19, 2021 20:18
@@ -303,6 +303,10 @@ export class Registry {
return tokens ? path.join(browserDirectory, ...tokens) : undefined;
}

exists(browserName: BrowserName): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is equal to the shouldRetain method below - let's just use it.

As a side note, shouldRetain is a pretty vague name - could you please rename it to something like isSupportedBrowser(browserName)?

@mxschmitt mxschmitt force-pushed the bugfix/host-dependencies-validation branch from 8d77591 to f42b6c4 Compare April 20, 2021 09:35
@mxschmitt mxschmitt force-pushed the bugfix/host-dependencies-validation branch from f42b6c4 to 4c2fc9d Compare April 20, 2021 14:04
@mxschmitt mxschmitt changed the title fix: host dependency validation fix: browser dependency validation Apr 20, 2021
@mxschmitt mxschmitt force-pushed the bugfix/host-dependencies-validation branch from 4c2fc9d to 23d1e8b Compare April 20, 2021 16:11
@mxschmitt mxschmitt merged commit 9cd89ae into microsoft:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [Regression] Host dependencies don't get validated anymore
3 participants