Skip to content

Commit

Permalink
fix: resolve navigation flakiness (#8768)
Browse files Browse the repository at this point in the history
Two main sources of flakiness addressed: 

1) we should dispose the lifecycle watcher after we waited for the navigation response (bad API? we need to refactor but I think it'd be valuable to stabilize tests first without too many changes).
2) we should wait for the navigation request's response if there is a navigation request in the watcher.

Closes #8644
  • Loading branch information
OrKoN committed Aug 10, 2022
1 parent c23cdb7 commit 2580347
Show file tree
Hide file tree
Showing 3 changed files with 502 additions and 9 deletions.
23 changes: 15 additions & 8 deletions src/common/FrameManager.ts
Expand Up @@ -227,11 +227,15 @@ export class FrameManager extends EventEmitter {
: watcher.sameDocumentNavigationPromise(),
]);
}
watcher.dispose();
if (error) {
throw error;

try {
if (error) {
throw error;
}
return await watcher.navigationResponse();
} finally {
watcher.dispose();
}
return await watcher.navigationResponse();

async function navigate(
client: CDPSession,
Expand Down Expand Up @@ -276,11 +280,14 @@ export class FrameManager extends EventEmitter {
watcher.sameDocumentNavigationPromise(),
watcher.newDocumentNavigationPromise(),
]);
watcher.dispose();
if (error) {
throw error;
try {
if (error) {
throw error;
}
return await watcher.navigationResponse();
} finally {
watcher.dispose();
}
return await watcher.navigationResponse();
}

async onAttachedToTarget(target: Target): Promise<void> {
Expand Down
26 changes: 25 additions & 1 deletion src/common/LifecycleWatcher.ts
Expand Up @@ -19,6 +19,8 @@ import {
addEventListener,
PuppeteerEventListener,
removeEventListeners,
DeferredPromise,
createDeferredPromise,
} from './util.js';
import {TimeoutError} from './Errors.js';
import {
Expand Down Expand Up @@ -100,6 +102,8 @@ export class LifecycleWatcher {
#hasSameDocumentNavigation?: boolean;
#swapped?: boolean;

#navigationResponseReceived?: DeferredPromise<void>;

constructor(
frameManager: FrameManager,
frame: Frame,
Expand Down Expand Up @@ -160,6 +164,11 @@ export class LifecycleWatcher {
NetworkManagerEmittedEvents.Request,
this.#onRequest.bind(this)
),
addEventListener(
this.#frameManager.networkManager(),
NetworkManagerEmittedEvents.Response,
this.#onResponse.bind(this)
),
];

this.#timeoutPromise = this.#createTimeoutPromise();
Expand All @@ -171,6 +180,20 @@ export class LifecycleWatcher {
return;
}
this.#navigationRequest = request;
this.#navigationResponseReceived?.reject(
new Error('New navigation request was received')
);
this.#navigationResponseReceived = createDeferredPromise();
if (request.response() !== null) {
this.#navigationResponseReceived?.resolve();
}
}

#onResponse(response: HTTPResponse): void {
if (this.#navigationRequest?._requestId !== response.request()._requestId) {
return;
}
this.#navigationResponseReceived?.resolve();
}

#onFrameDetached(frame: Frame): void {
Expand All @@ -185,7 +208,8 @@ export class LifecycleWatcher {
}

async navigationResponse(): Promise<HTTPResponse | null> {
// We may need to wait for ExtraInfo events before the request is complete.
// Continue with a possibly null response.
await this.#navigationResponseReceived?.catch(() => {});
return this.#navigationRequest ? this.#navigationRequest.response() : null;
}

Expand Down

0 comments on commit 2580347

Please sign in to comment.