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: wait page load #778

Merged
merged 1 commit into from
Sep 19, 2023
Merged

feat: wait page load #778

merged 1 commit into from
Sep 19, 2023

Conversation

KuznetsovRoman
Copy link
Member

Что сделано

Добавил метод openAndWait:

  • Открыть страницу
  • Подождать появления селекторов
  • Подождать предикат
  • Подождать неактивности сети
  • Кинуть ошибку при ошибке загрузки ресурса

@KuznetsovRoman KuznetsovRoman force-pushed the HERMIONE-772.wait_page_load branch 2 times, most recently from e6afbd0 to e7fc9b4 Compare July 20, 2023 08:09
timeout = openAndWaitOpts?.timeout || config?.pageLoadTimeout,
}: WaitOpts = {},
): Promise<string | void> {
waitNetworkIdle &&= isChrome || isCDP;
Copy link
Member Author

Choose a reason for hiding this comment

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

Отключаем waitNetworkIdle, если использование CDP Interceptor невозможно

Comment on lines 62 to 68
if (!uri || uri === emptyPageUrl) {
return session.url(uri);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

На пустых страницах не нужно ничего ждать.

return session.url(uri);
}

const selectors = typeof selector === "string" ? [selector] : selector;
Copy link
Member Author

Choose a reason for hiding this comment

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

typeof вместо _.isString, чтобы у selectors правильно вывелся тип: string[]


const selectors = typeof selector === "string" ? [selector] : selector;

const pageLoader = new PageLoader(session, {
Copy link
Member Author

Choose a reason for hiding this comment

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

Основная логика сосредоточена в этом классе

return;
}

const shouldIgnore = isMatchPatterns(openAndWaitOpts?.ignoreNetworkErrorsPatterns, match.url);
Copy link
Member Author

Choose a reason for hiding this comment

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

Помимо shouldThrowError, добавил ignoreNetworkErrorPatterns: чтобы удобно было игнорировать всякие ошибки запросов к счетчикам, которые браузер считает картинками

});

pageLoader.load(() => session.url(uri)).then(checkLoaded);
}).finally(() => pageLoader.unsubscribe());
Copy link
Member Author

Choose a reason for hiding this comment

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

очищаем mock после выполнения команды. Так puppeteer работает стабильнее, немного быстрее, не утекает память.

@@ -240,6 +240,16 @@ function buildBrowserOptions(defaultFactory, extra) {
},
}),

openAndWaitOpts: option({
Copy link
Member Author

Choose a reason for hiding this comment

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

Возможность указать дефолты через конфиг. Например, чтобы были единые таймауты, плюс тут удобно указать игнорируемые паттерны метрики

}

public async load(goToPage: () => Promise<string | void>): Promise<void> {
await this.initMock();
Copy link
Member Author

Choose a reason for hiding this comment

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

Начинаем слушать сеть до загрузки страницы

Comment on lines 45 to 47
this.launchSelectorsPromise();
this.launchPredicatePromise();
this.launchNetworkPromise();
Copy link
Member Author

Choose a reason for hiding this comment

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

после загрузки страницы, параллельно запускаем три промиса с единым таймаутом:

  • selectorsPromise: ждет появления всех селекторов, по таймауту кидает ошибку
  • predicatePromise: ждет истинности предиката, по таймауту кидает ошибку
  • networkPromise: нужен для того, чтобы по таймауту кинуть ошибку (либо заэмиттить эвент о том, что сеть уже неактивна, если никаких запросов и не было)

Comment on lines +58 to +59
const element = await this.session.$(selector);
await element.waitForExist({ timeout: this.timeout });
Copy link
Member Author

Choose a reason for hiding this comment

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

Запись в две строчки вместо чейнинга упрощает тестирование

Comment on lines +109 to +113
this.mock = await this.session.mock("**").catch(() => {
logger.warn(`PageLoader: Could not create CDP interceptor`);

return 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.

На практике такого не встречал, но если не удастся создать CDP interceptor, то падение теста по этой причине принесет больше проблем, чем пользы, поэтому если не удалось, игнорируем

let pendingRequests = 0;
let pendingIdleTimeout: NodeJS.Timeout;
this.mock.on("request", () => {
this.totalRequests++;
Copy link
Member Author

Choose a reason for hiding this comment

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

Счетчик полного количества запросов нужен, чтобы по networkIdleTimeout обозначить, что сеть пассивная, если ни одного запроса так и не было (launchNetworkPromise)

private mock?: Mock | null;
private selectors: string[];
private predicate?: () => boolean | Promise<boolean>;
private timeout: number;
Copy link
Member Author

Choose a reason for hiding this comment

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

Сколько мы готовы потратить времени на итоговую прогрузку страницы (селекторы, предикат, неактивность сети)

private predicate?: () => boolean | Promise<boolean>;
private timeout: number;
private waitNetworkIdle: boolean;
private waitNetworkIdleTimeout: number;
Copy link
Member Author

Choose a reason for hiding this comment

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

спустя какое время отсутствия запросов считать, что страница прогрузилась

Comment on lines +12 to +17
type SessionOrigin = ReturnType<typeof mkSessionStubOrigin_>;
type Session = SessionOrigin & { openAndWait(uri: string, opts: Record<string, unknown>): Promise<void> };

const mkSessionStub_ = (): Session => {
return mkSessionStubOrigin_() as Session;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Создаем правильно типизированный mkSessionStub_, у которого есть метод openAndWait

sandbox.restore();
});

const mkPromiseCheck_ =
Copy link
Member Author

Choose a reason for hiding this comment

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

функция-конструктор проверок ниже: resolvedAfter_, rejectedAfter_, notResolvedAfter_, notRejectedAfer_

restore: SinonStub;
};

const waitUntilMock = (condition: () => boolean, opts?: WaitForOptions): Promise<void> => {
Copy link
Member Author

Choose a reason for hiding this comment

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

waitUntil у session застабано. Чтобы проверить логику с предикатами, нужно вернуть waitUntil нормальное поведение

@KuznetsovRoman KuznetsovRoman force-pushed the HERMIONE-772.wait_page_load branch 3 times, most recently from d059153 to 1928186 Compare September 7, 2023 13:22
});

if (!this.mock) {
this.markNetworkIdle();
Copy link
Member Author

Choose a reason for hiding this comment

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

markNetworkIdle вернет true, если изначально сеть была активной и стала пассивной после вызова этой функции. Здесь нам не нужно ее возвращаемое значение, потому что оно всегда будет true.

Comment on lines +97 to +101
setTimeout(() => {
if (!this.totalRequests) {
this.markNetworkIdle();
}
}, this.waitNetworkIdleTimeout);
Copy link
Member Author

Choose a reason for hiding this comment

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

Случай "на странице нет никаких запросов", помечаем сеть пассивной по таймауту

Comment on lines +91 to +96
setTimeout(() => {
const markSuccess = this.markNetworkIdle();
if (markSuccess) {
logger.warn(`PageLoader: Network idle timeout`);
}
}, this.timeout);
Copy link
Member Author

Choose a reason for hiding this comment

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

Тут - единственное место, где мы смотрим на значение. Читается это как "Если раньше сеть была активной, и мы помечаем ее пассивной по таймауту, то пишем лог. Иначе - не пишем". Это возвращаемое значение нужно только тут, и только для того, чтоб не писать лишний лог.

src/browser/commands/openAndWait.ts Outdated Show resolved Hide resolved
src/browser/commands/openAndWait.ts Show resolved Hide resolved
src/config/browser-options.js Outdated Show resolved Hide resolved
src/utils/page-loader.ts Outdated Show resolved Hide resolved
src/utils/page-loader.ts Outdated Show resolved Hide resolved
src/utils/page-loader.ts Show resolved Hide resolved
@KuznetsovRoman KuznetsovRoman merged commit 9e7c1a3 into master Sep 19, 2023
2 checks passed
@KuznetsovRoman KuznetsovRoman deleted the HERMIONE-772.wait_page_load branch September 19, 2023 21:56
import { Matches } from "webdriverio";
import PageLoader from "../../utils/page-loader";

interface Browser {
Copy link
Member

Choose a reason for hiding this comment

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

ну этот интерфейс явно должен лежать где-то рядом с самим браузером, а не в реализации одной из его команд

Copy link
Member Author

Choose a reason for hiding this comment

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

Помню, что описывал его тут отдельно, потому что была проблема с типами webdriverio по части url.
Но сейчас оно реализовано отдельной командой, и этих проблем нет, спокойно будет работать, если заменить этот блок на import type Browser from "../browser";

parseCli: JSON.parse,
validate: value => utils.assertOptionalObject(value, "openAndWaitOpts"),
map: value => {
return value === defaults.openAndWaitOpts ? value : { ...defaults.openAndWaitOpts, ...value };
Copy link
Member

Choose a reason for hiding this comment

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

почему здесь просто не оставить вторую часть условия?

setTimeout(() => {
const markSuccess = this.markNetworkIdle();
if (markSuccess) {
logger.warn(`PageLoader: Network idle timeout`);
Copy link
Member

Choose a reason for hiding this comment

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

для чего здесь именно warn? и само сообщение мне ни о чем не говорит

Copy link
Member Author

Choose a reason for hiding this comment

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

Это сообщение поважнее обычного лога
Свидетельствует о том, что "что-то пошло не так", но при этом - не так критично, как ошибка.
Само сообщение - один из аргументов называется waitNetworkIdle, и есть еще опция timeout. Вот это значит, что по таймауту не дождались. Можно заменить на Network was never idle in ${timeout} ms

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.

3 participants