Skip to content

Commit

Permalink
Fix the timeout logic in the waitForEvent integration test helper f…
Browse files Browse the repository at this point in the history
…unction

Debugging #17931, by printing all parts of the event lifecycle including
timestamps, uncovered that some events for which a timeout was logged
actually did get triggered correctly in the browser. Going over the code
and discovering https://stackoverflow.com/questions/47107465/puppeteer-how-to-listen-to-object-events#comment117661238_65534026
showed what went wrong: if the event we wait for is triggered then
`Promise.race` resolves, but that doesn't automatically cancel the
timeout. The tests didn't fail on this because `Promise.race` resolved
correctly, but slightly later once the timeout was reached we would see
spurious log lines about timeouts for the already-triggered events.

This commit fixes the issue by canceling the timeout if the event we're
waiting for has triggered.
  • Loading branch information
timvandermeij committed Jun 24, 2024
1 parent e16707d commit 51dcd6a
Showing 1 changed file with 9 additions and 3 deletions.
12 changes: 9 additions & 3 deletions test/integration/test_utils.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -189,16 +189,22 @@ async function getSpanRectFromText(page, pageNumber, text) {
async function waitForEvent(page, eventName, timeout = 5000) {
const handle = await page.evaluateHandle(
(name, timeOut) => {
let callback = null;
let callback = null,
timeoutId = null;
return [
Promise.race([
new Promise(resolve => {
// add event listener and wait for event to fire before returning
callback = () => resolve(false);
callback = () => {
if (timeoutId) {
clearTimeout(timeoutId);
}
resolve(false);
};
document.addEventListener(name, callback, { once: true });
}),
new Promise(resolve => {
setTimeout(() => {
timeoutId = setTimeout(() => {
document.removeEventListener(name, callback);
resolve(true);
}, timeOut);
Expand Down

0 comments on commit 51dcd6a

Please sign in to comment.