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

feat: auto ubuntu packages download for local browsers #1036

Merged
merged 13 commits into from
Dec 17, 2024
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
fix: remove registry v0 support
  • Loading branch information
KuznetsovRoman committed Dec 17, 2024
commit bde19e066e69321d644c09e784e179bae47b38a6
46 changes: 18 additions & 28 deletions src/browser-installer/registry/index.ts
Original file line number Diff line number Diff line change
@@ -13,17 +13,22 @@ import {
type SupportedBrowser,
type SupportedDriver,
type DownloadProgressCallback,
type BinaryKey,
type BinaryName,
type OsName,
type OsVersion,
type OsPackagesKey,
type VersionToPathMap,
type RegistryFileContents,
} from "../utils";
import { getFirefoxBuildId } from "../firefox/utils";
import logger from "../../utils/logger";

type VersionToPathMap = Record<string, string | Promise<string>>;
type BinaryName = Exclude<SupportedBrowser | SupportedDriver, SupportedBrowser & SupportedDriver>;
type BinaryKey = `${BinaryName}_${BrowserPlatform}`;
type OsName = string;
type OsVersion = string;
type OsPackagesKey = `${OsName}_${OsVersion}`;
export type RegistryFileContents = {
binaries: Record<BinaryKey, VersionToPathMap>;
osPackages: Record<OsPackagesKey, string | Promise<string>>;
meta: { version: number };
};

const getRegistryBinaryKey = (name: BinaryName, platform: BrowserPlatform): BinaryKey => `${name}_${platform}`;
const getRegistryOsPackagesKey = (name: OsName, version: OsVersion): OsPackagesKey => `${name}_${version}`;

@@ -245,28 +250,13 @@ class Registry {
}

private readRegistry(): RegistryFileContents {
const registry: RegistryFileContents = {
binaries: {} as Record<BinaryKey, VersionToPathMap>,
osPackages: {} as Record<OsPackagesKey, string>,
meta: { version: 1 },
};

let fsData: Record<string, unknown>;

if (fs.existsSync(this.registryPath)) {
fsData = fs.readJSONSync(this.registryPath);

const isRegistryV0 = fsData && !fsData.meta;
const isRegistryWithVersion = typeof _.get(fsData, "meta.version") === "number";
const registry: RegistryFileContents = fs.existsSync(this.registryPath)
? fs.readJSONSync(this.registryPath)
: {};

if (isRegistryWithVersion) {
return fsData as RegistryFileContents;
}

if (isRegistryV0) {
registry.binaries = fsData as Record<BinaryKey, VersionToPathMap>;
}
}
registry.binaries ||= {} as Record<BinaryKey, VersionToPathMap>;
registry.osPackages ||= {} as Record<OsPackagesKey, string>;
registry.meta ||= { version: 1 };

return registry;
}
Original file line number Diff line number Diff line change
@@ -2,7 +2,8 @@ import path from "path";
import fs from "fs";
import _ from "lodash";
import { installBrowser } from "../..";
import { getRegistryPath, type RegistryFileContents } from "../../utils";
import { getRegistryPath } from "../../utils";
import type { RegistryFileContents } from "../../registry";
import type { BrowserWithVersion } from "./utils";

const getRegistryBinaryPaths = (registry: RegistryFileContents): string[] => {
14 changes: 1 addition & 13 deletions src/browser-installer/utils.ts
Original file line number Diff line number Diff line change
@@ -29,18 +29,6 @@ export const Driver = {
export type SupportedBrowser = (typeof Browser)[keyof typeof Browser];
export type SupportedDriver = (typeof Driver)[keyof typeof Driver];

export type VersionToPathMap = Record<string, string | Promise<string>>;
export type BinaryName = Exclude<SupportedBrowser | SupportedDriver, SupportedBrowser & SupportedDriver>;
export type BinaryKey = `${BinaryName}_${BrowserPlatform}`;
export type OsName = string;
export type OsVersion = string;
export type OsPackagesKey = `${OsName}_${OsVersion}`;
export type RegistryFileContents = {
binaries: Record<BinaryKey, VersionToPathMap>;
osPackages: Record<OsPackagesKey, string | Promise<string>>;
meta: { version: number };
};

export const getNormalizedBrowserName = (
Copy link
Member Author

Choose a reason for hiding this comment

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

As now we dont map "browserName" to "driverName" for "runBrowserDriver", we dont need "getDriverNameForBrowserName", and instead i created this function, so in "installBrowser" we can be sure it only receives valid "browserName" of SupportedBrowser type

Copy link
Member Author

Choose a reason for hiding this comment

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

This way we can handle cases with invalid browser name outside of "install", so we know, what exactly user wanted to do, and we can provide relevant error message:

  • invalid browser name was used in order to run local browser
  • invalid browser name was used in order to download a browser

browserName?: string,
): Exclude<SupportedBrowser, typeof Browser.CHROMIUM> | null => {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need exclude here?

Copy link
Member Author

@KuznetsovRoman KuznetsovRoman Dec 16, 2024

Choose a reason for hiding this comment

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

We dont need to, i just wanted to show "chromium can't be returned here", because chromium maps to chrome, as installing "chrome" and "chromium" has a single entry point, being "chrome", as "chromium" is not a valid W3C browserName

@@ -147,7 +135,7 @@ const getDriversDir = (): string => path.join(getCacheDir(), "drivers");
const getDriverDir = (driverName: string, driverVersion: string): string =>
path.join(getDriversDir(), driverName, driverVersion);

export const getOsPackagesDir = (osName: OsName, osVersion: OsVersion): string =>
export const getOsPackagesDir = (osName: string, osVersion: string): string =>
path.join(getCacheDir(), "packages", osName, osVersion);

export const getGeckoDriverDir = (driverVersion: string): string =>
8 changes: 2 additions & 6 deletions test/src/browser-installer/registry.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
import proxyquire from "proxyquire";
import sinon, { type SinonStub } from "sinon";
import type RegistryType from "../../../src/browser-installer/registry";
import {
Browser,
Driver,
type DownloadProgressCallback,
type RegistryFileContents,
} from "../../../src/browser-installer/utils";
import type { RegistryFileContents } from "../../../src/browser-installer/registry";
import { Browser, Driver, type DownloadProgressCallback } from "../../../src/browser-installer/utils";
import { BrowserPlatform } from "@puppeteer/browsers";
import type { PartialDeep } from "type-fest";

26 changes: 19 additions & 7 deletions test/src/browser-installer/run.ts
Original file line number Diff line number Diff line change
@@ -10,14 +10,17 @@ describe("browser-installer/run", () => {

let installBrowserStub: SinonStub;
let runChromeDriverStub: SinonStub;
let runGeckoDriverStub: SinonStub;

beforeEach(() => {
installBrowserStub = sandbox.stub();
runChromeDriverStub = sandbox.stub();
runGeckoDriverStub = sandbox.stub();

runBrowserDriver = proxyquire.noCallThru()("../../../src/browser-installer/run", {
"./install": { installBrowser: installBrowserStub },
"./chrome": { runChromeDriver: runChromeDriverStub },
"./firefox": { runGeckoDriver: runGeckoDriverStub },
}).runBrowserDriver;
});

@@ -31,14 +34,23 @@ describe("browser-installer/run", () => {
});
});

[Browser.CHROME, Browser.EDGE, Browser.FIREFOX].forEach(browser => {
it(`should try to install ${browser} before running its driver`, async () => {
await runBrowserDriver(browser, "some-version");
it(`should try to install chrome before running its driver`, async () => {
await runBrowserDriver(Browser.CHROME, "some-version");

assert.calledOnceWith(installBrowserStub, browser, "some-version", {
shouldInstallWebDriver: true,
shouldInstallUbuntuPackages: true,
});
assert.calledOnceWith(installBrowserStub, Browser.CHROME, "some-version", {
shouldInstallWebDriver: true,
shouldInstallUbuntuPackages: true,
});
assert.callOrder(installBrowserStub, runChromeDriverStub);
});

it(`should try to install firefox before running its driver`, async () => {
await runBrowserDriver(Browser.FIREFOX, "some-version");

assert.calledOnceWith(installBrowserStub, Browser.FIREFOX, "some-version", {
shouldInstallWebDriver: true,
shouldInstallUbuntuPackages: true,
});
assert.callOrder(installBrowserStub, runGeckoDriverStub);
});
});
Loading