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: ability to disable tests isolation #821

Merged
merged 1 commit into from
Dec 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1135,7 +1135,7 @@ Ability to choose different datacenters for run in cloud service. Default value
Ability to run headless browser in cloud service. Default value is `null`.

#### isolation
Ability to execute tests in isolated clean-state environment ([incognito browser context](https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createBrowserContext)). It means that `testsPerSession` can be set to `Infinity` in order to speed up tests execution and save browser resources. Currently works only in chrome@93 and higher. Default value is `false`, but `true` for chrome@93 and higher.
Ability to execute tests in isolated clean-state environment ([incognito browser context](https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createBrowserContext)). It means that `testsPerSession` can be set to `Infinity` in order to speed up tests execution and save browser resources. Currently works only in chrome@93 and higher. Default value is `null`, but `true` for chrome@93 and higher.

### system

Expand Down
6 changes: 3 additions & 3 deletions src/config/browser-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,15 +319,15 @@ function buildBrowserOptions(defaultFactory, extra) {
defaultValue: defaultFactory("isolation"),
parseCli: value => utils.parseBoolean(value, "isolation"),
parseEnv: value => utils.parseBoolean(value, "isolation"),
validate: is("boolean", "isolation"),
validate: value => _.isNull(value) || is("boolean", "isolation")(value),
map: (value, config, currentNode, meta) => {
if (meta.isSpecified) {
if (meta.isSetByUser || !_.isNull(value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Тут ошибся и заюзал старое название поля isSpecified (раньше так называлось). Но позже мы его переименовали в isSetByUser. Поправил.

Кроме этого дефолт переделал в null иначе не получается корректно обработать значение.
Тут сначала map выполняется для top level конфига, а потом для каждого браузера. И проблема в том, что если isolation указать в top level, то при его обработке флаг isSetByUser будет выставлен корректно. Но затем map будет вызван для каждого браузера и для него isSetByUser: false и при этом дефолтное значение прилетает из топ левела (если оно было указано). Поэтому тут приходится писать чуть сложнее решение.

Copy link
Member

@KuznetsovRoman KuznetsovRoman Dec 27, 2023

Choose a reason for hiding this comment

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

Печально, что приходится к boolean значениям примешивать null, но дешевых вариантов "как сделать тут лучше" я не вижу.

Copy link
Member Author

Choose a reason for hiding this comment

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

Я пока более простого решения не нашел. По дефолту в конфиге браузера у юзера все равно будет валидный boolean. Так как capabilities при разборе конфига браузера - обязательное значение. Т.е. всегда будем проваливаться в isSupportIsolation, который возвращает boolean.

return value;
}

const caps = _.get(currentNode, "desiredCapabilities");

return caps && isSupportIsolation(caps.browserName, caps.browserVersion) ? true : value;
return caps ? isSupportIsolation(caps.browserName, caps.browserVersion) : value;
},
}),
});
Expand Down
2 changes: 1 addition & 1 deletion src/config/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ module.exports = {
key: null,
region: null,
headless: null,
isolation: false,
isolation: null,
Copy link
Member Author

Choose a reason for hiding this comment

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

Если оставить false, то я при разборе опции для браузера не смогу понять это ее выставил юзер в top level конфиге или это дефолтное значение.

};

module.exports.configPaths = [".hermione.conf.ts", ".hermione.conf.js"];
40 changes: 40 additions & 0 deletions test/src/config/browser-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,46 @@ describe("config browser-options", () => {

assert.isFalse(config.browsers.b1.isolation);
});

describe("should set to 'false' by user even if browser support isolation", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Добавил тестов, которые не работали со старым кодом.

it("in top level config", () => {
const readConfig = {
isolation: false,
browsers: {
b1: mkBrowser_({
desiredCapabilities: {
browserName: "chrome",
browserVersion: "101.0",
},
}),
},
};
Config.read.returns(readConfig);

const config = createConfig();

assert.isFalse(config.browsers.b1.isolation);
});

it("in browser config", () => {
const readConfig = {
browsers: {
b1: mkBrowser_({
isolation: false,
desiredCapabilities: {
browserName: "chrome",
browserVersion: "101.0",
},
}),
},
};
Config.read.returns(readConfig);

const config = createConfig();

assert.isFalse(config.browsers.b1.isolation);
});
});
});

describe("saveHistoryMode", () => {
Expand Down
Loading