From 55ba4aa66aa6c1da1f1bdb0f9c9a5654a2c78c74 Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Fri, 21 Jun 2024 20:37:01 +0200 Subject: [PATCH 1/2] Refactor the copy/paste logic in the integration tests The integration tests are currently not consistent in how they do copy/pasting: some tests use the `kbCopy`/`kbPaste` functions with waiting for the event inline, some have their own helper function to combine those actions and some even call `kbCopy`/`kbPaste` without waiting for the event at all (which can cause intermittent failures). This commit fixes the issues by providing a set of four helper functions that all tests use and that abstract e.g. waiting for the event away from the caller. This makes the invididual tests simpler and consistent, reduces code duplication and fixes possible intermittent failures due to not waiting for events to trigger. --- test/integration/copy_paste_spec.mjs | 12 +-- test/integration/freetext_editor_spec.mjs | 98 +++++++++-------------- test/integration/stamp_editor_spec.mjs | 23 +++--- test/integration/test_utils.mjs | 21 ++++- 4 files changed, 69 insertions(+), 85 deletions(-) diff --git a/test/integration/copy_paste_spec.mjs b/test/integration/copy_paste_spec.mjs index 6c18929d9..94d6be9a7 100644 --- a/test/integration/copy_paste_spec.mjs +++ b/test/integration/copy_paste_spec.mjs @@ -15,7 +15,7 @@ import { closePages, - kbCopy, + copy, kbSelectAll, loadAndWait, mockClipboard, @@ -55,10 +55,7 @@ describe("Copy and paste", () => { ); await selectAll(page); - const promise = waitForEvent(page, "copy"); - await kbCopy(page); - await promise; - + await copy(page); await page.waitForFunction( `document.querySelector('#viewerContainer').style.cursor !== "wait"` ); @@ -159,10 +156,7 @@ describe("Copy and paste", () => { ); await selectAll(page); - const promise = waitForEvent(page, "copy"); - await kbCopy(page); - await promise; - + await copy(page); await page.waitForFunction( `document.querySelector('#viewerContainer').style.cursor !== "wait"` ); diff --git a/test/integration/freetext_editor_spec.mjs b/test/integration/freetext_editor_spec.mjs index d7afe96ab..6427ac28c 100644 --- a/test/integration/freetext_editor_spec.mjs +++ b/test/integration/freetext_editor_spec.mjs @@ -16,6 +16,8 @@ import { awaitPromise, closePages, + copy, + copyToClipboard, createPromise, dragAndDropAnnotation, firstPageOnTop, @@ -30,21 +32,19 @@ import { kbBigMoveLeft, kbBigMoveRight, kbBigMoveUp, - kbCopy, kbGoToBegin, kbGoToEnd, kbModifierDown, kbModifierUp, - kbPaste, kbRedo, kbSelectAll, kbUndo, loadAndWait, + paste, pasteFromClipboard, scrollIntoView, switchToEditor, waitForAnnotationEditorLayer, - waitForEvent, waitForSelectedEditor, waitForSerialized, waitForStorageEntries, @@ -53,16 +53,6 @@ import { } from "./test_utils.mjs"; import { PNG } from "pngjs"; -const copyPaste = async page => { - let promise = waitForEvent(page, "copy"); - await kbCopy(page); - await promise; - - promise = waitForEvent(page, "paste"); - await kbPaste(page); - await promise; -}; - const selectAll = async page => { await kbSelectAll(page); await page.waitForFunction( @@ -187,7 +177,8 @@ describe("FreeText Editor", () => { ); await waitForSelectedEditor(page, getEditorSelector(0)); - await copyPaste(page); + await copy(page); + await paste(page); await page.waitForSelector(getEditorSelector(1), { visible: true, }); @@ -203,7 +194,8 @@ describe("FreeText Editor", () => { expect(pastedContent).withContext(`In ${browserName}`).toEqual(content); - await copyPaste(page); + await copy(page); + await paste(page); await page.waitForSelector(getEditorSelector(2), { visible: true, }); @@ -263,7 +255,8 @@ describe("FreeText Editor", () => { ); await waitForSelectedEditor(page, getEditorSelector(3)); - await copyPaste(page); + await copy(page); + await paste(page); await page.waitForSelector(getEditorSelector(4), { visible: true, }); @@ -276,9 +269,7 @@ describe("FreeText Editor", () => { ); for (let i = 0; i < 2; i++) { - const promise = waitForEvent(page, "paste"); - await kbPaste(page); - await promise; + await paste(page); await page.waitForSelector(getEditorSelector(5 + i)); } @@ -597,7 +588,8 @@ describe("FreeText Editor", () => { .withContext(`In ${browserName}`) .toEqual([0, 1, 3]); - await copyPaste(page); + await copy(page); + await paste(page); await page.waitForSelector(getEditorSelector(6), { visible: true, }); @@ -1275,7 +1267,8 @@ describe("FreeText Editor", () => { ); await waitForSelectedEditor(page, getEditorSelector(1)); - await copyPaste(page); + await copy(page); + await paste(page); await page.waitForSelector(getEditorSelector(6), { visible: true, }); @@ -3425,14 +3418,11 @@ describe("FreeText Editor", () => { ); await select(0); - await pasteFromClipboard( - page, - { - "text/html": "Bold Foo", - "text/plain": "Foo", - }, - `${editorSelector} .internal` - ); + await copyToClipboard(page, { + "text/html": "Bold Foo", + "text/plain": "Foo", + }); + await pasteFromClipboard(page, `${editorSelector} .internal`); let lastText = data; @@ -3442,14 +3432,11 @@ describe("FreeText Editor", () => { expect(text).withContext(`In ${browserName}`).toEqual(lastText); await select(3); - await pasteFromClipboard( - page, - { - "text/html": "Bold Bar
Oof", - "text/plain": "Bar\nOof", - }, - `${editorSelector} .internal` - ); + await copyToClipboard(page, { + "text/html": "Bold Bar
Oof", + "text/plain": "Bar\nOof", + }); + await pasteFromClipboard(page, `${editorSelector} .internal`); await waitForTextChange(lastText, editorSelector); text = await getText(editorSelector); @@ -3457,13 +3444,8 @@ describe("FreeText Editor", () => { expect(text).withContext(`In ${browserName}`).toEqual(lastText); await select(0); - await pasteFromClipboard( - page, - { - "text/html": "basic html", - }, - `${editorSelector} .internal` - ); + await copyToClipboard(page, { "text/html": "basic html" }); + await pasteFromClipboard(page, `${editorSelector} .internal`); // Nothing should change, so it's hard to wait on something. // eslint-disable-next-line no-restricted-syntax @@ -3477,15 +3459,12 @@ describe("FreeText Editor", () => { const prevHTML = await getHTML(); // Try to paste an image. - await pasteFromClipboard( - page, - { - "image/png": - // 1x1 transparent png. - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==", - }, - `${editorSelector} .internal` - ); + await copyToClipboard(page, { + "image/png": + // 1x1 transparent png. + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==", + }); + await pasteFromClipboard(page, `${editorSelector} .internal`); // Nothing should change, so it's hard to wait on something. // eslint-disable-next-line no-restricted-syntax @@ -3505,14 +3484,11 @@ describe("FreeText Editor", () => { }); const fooBar = "Foo\nBar\nOof"; - await pasteFromClipboard( - page, - { - "text/html": "html", - "text/plain": fooBar, - }, - `${editorSelector} .internal` - ); + await copyToClipboard(page, { + "text/html": "html", + "text/plain": fooBar, + }); + await pasteFromClipboard(page, `${editorSelector} .internal`); await waitForTextChange("", editorSelector); text = await getText(editorSelector); diff --git a/test/integration/stamp_editor_spec.mjs b/test/integration/stamp_editor_spec.mjs index b8243f17e..66b2735bc 100644 --- a/test/integration/stamp_editor_spec.mjs +++ b/test/integration/stamp_editor_spec.mjs @@ -17,6 +17,8 @@ import { applyFunctionToEditor, awaitPromise, closePages, + copy, + copyToClipboard, getEditorDimensions, getEditorSelector, getFirstSerialized, @@ -24,11 +26,10 @@ import { getSerialized, kbBigMoveDown, kbBigMoveRight, - kbCopy, - kbPaste, kbSelectAll, kbUndo, loadAndWait, + paste, pasteFromClipboard, scrollIntoView, serializeBitmapDimensions, @@ -78,12 +79,10 @@ const copyImage = async (page, imagePath, number) => { const data = fs .readFileSync(path.join(__dirname, imagePath)) .toString("base64"); - await pasteFromClipboard( - page, - { "image/png": `data:image/png;base64,${data}` }, - "", - 500 - ); + + await copyToClipboard(page, { "image/png": `data:image/png;base64,${data}` }); + await pasteFromClipboard(page); + await waitForImage(page, getEditorSelector(number)); }; @@ -570,13 +569,13 @@ describe("Stamp Editor", () => { await page1.click("#editorStamp"); await copyImage(page1, "../images/firefox_logo.png", 0); - await kbCopy(page1); + await copy(page1); const [, page2] = pages2[i]; await page2.bringToFront(); await page2.click("#editorStamp"); - await kbPaste(page2); + await paste(page2); await waitForImage(page2, getEditorSelector(0)); } @@ -831,8 +830,8 @@ describe("Stamp Editor", () => { ); await page.waitForSelector(`${getEditorSelector(0)} .altText.done`); - await kbCopy(page); - await kbPaste(page); + await copy(page); + await paste(page); await page.waitForSelector(`${getEditorSelector(1)} .altText.done`); await waitForSerialized(page, 2); diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 39fe88bd4..3d957680f 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -292,7 +292,13 @@ async function mockClipboard(pages) { ); } -async function pasteFromClipboard(page, data, selector, timeout = 100) { +async function copy(page) { + const promise = waitForEvent(page, "copy"); + await kbCopy(page); + await promise; +} + +async function copyToClipboard(page, data) { await page.evaluate(async dat => { const items = Object.create(null); for (const [type, value] of Object.entries(dat)) { @@ -305,7 +311,15 @@ async function pasteFromClipboard(page, data, selector, timeout = 100) { } await navigator.clipboard.write([new ClipboardItem(items)]); }, data); +} +async function paste(page) { + const promise = waitForEvent(page, "paste"); + await kbPaste(page); + await promise; +} + +async function pasteFromClipboard(page, selector = null) { const validator = e => e.clipboardData.items.length !== 0; let hasPasteEvent = false; while (!hasPasteEvent) { @@ -634,6 +648,8 @@ export { clearInput, closePages, closeSinglePage, + copy, + copyToClipboard, createPromise, dragAndDropAnnotation, firstPageOnTop, @@ -654,7 +670,6 @@ export { kbBigMoveLeft, kbBigMoveRight, kbBigMoveUp, - kbCopy, kbDeleteLastWord, kbFocusNext, kbFocusPrevious, @@ -662,12 +677,12 @@ export { kbGoToEnd, kbModifierDown, kbModifierUp, - kbPaste, kbRedo, kbSelectAll, kbUndo, loadAndWait, mockClipboard, + paste, pasteFromClipboard, scrollIntoView, serializeBitmapDimensions, From 7128b95d29a9b802746b61ee9ecd7ef868b2141e Mon Sep 17 00:00:00 2001 From: Tim van der Meij Date: Sun, 23 Jun 2024 16:10:27 +0200 Subject: [PATCH 2/2] Fix a race condition involving the `waitForEvent` integration test helper function Debugging #17931 uncovered a race condition in the way we use the `waitForEvent` function. Currently the following happens: 1. We call `waitForEvent`, which starts execution of the function body and immediately returns a promise. 2. We do the action that triggers the event. 3. We await the promise, which resolves if the event is triggered or the timeout is reached. The problem is in step 1: function body execution has started, but not necessarily completed. Given that we don't await the promise, we immediately trigger step 2 and it's not unlikely that the event we trigger arrives before the event listener is actually registered in the function body of `waitForEvent` (which is slower because it needs to be evaluated in the page context and there is some other logic before the actual `addEventListener` call). This commit fixes the issue by passing the action to `waitForEvent` as a callback so `waitForEvent` itself can call it once it's safe to do so. This should make sure that we always register the event listener before triggering the event, and because we shouldn't miss events anymore we can also remove the retry logic for pasting. --- test/integration/copy_paste_spec.mjs | 8 ++++-- test/integration/test_utils.mjs | 41 ++++++++++++++++------------ 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/test/integration/copy_paste_spec.mjs b/test/integration/copy_paste_spec.mjs index 94d6be9a7..cb5ae3ab9 100644 --- a/test/integration/copy_paste_spec.mjs +++ b/test/integration/copy_paste_spec.mjs @@ -23,9 +23,11 @@ import { } from "./test_utils.mjs"; const selectAll = async page => { - const promise = waitForEvent(page, "selectionchange"); - await kbSelectAll(page); - await promise; + await waitForEvent({ + page, + eventName: "selectionchange", + action: () => kbSelectAll(page), + }); await page.waitForFunction(() => { const selection = document.getSelection(); diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 3d957680f..1c8d39d0d 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -186,13 +186,14 @@ async function getSpanRectFromText(page, pageNumber, text) { ); } -async function waitForEvent( +async function waitForEvent({ page, eventName, + action, selector = null, validator = null, - timeout = 5000 -) { + timeout = 5000, +}) { const handle = await page.evaluateHandle( (name, sel, validate, timeOut) => { let callback = null, @@ -227,13 +228,15 @@ async function waitForEvent( validator ? validator.toString() : null, timeout ); + + await action(); + const success = await awaitPromise(handle); if (success === null) { console.log(`waitForEvent: ${eventName} didn't trigger within the timeout`); } else if (!success) { console.log(`waitForEvent: ${eventName} triggered, but validation failed`); } - return success; } async function waitForStorageEntries(page, nEntries) { @@ -293,9 +296,11 @@ async function mockClipboard(pages) { } async function copy(page) { - const promise = waitForEvent(page, "copy"); - await kbCopy(page); - await promise; + await waitForEvent({ + page, + eventName: "copy", + action: () => kbCopy(page), + }); } async function copyToClipboard(page, data) { @@ -314,20 +319,22 @@ async function copyToClipboard(page, data) { } async function paste(page) { - const promise = waitForEvent(page, "paste"); - await kbPaste(page); - await promise; + await waitForEvent({ + page, + eventName: "paste", + action: () => kbPaste(page), + }); } async function pasteFromClipboard(page, selector = null) { const validator = e => e.clipboardData.items.length !== 0; - let hasPasteEvent = false; - while (!hasPasteEvent) { - // We retry to paste if nothing has been pasted before the timeout. - const promise = waitForEvent(page, "paste", selector, validator); - await kbPaste(page); - hasPasteEvent = await promise; - } + await waitForEvent({ + page, + eventName: "paste", + action: () => kbPaste(page), + selector, + validator, + }); } async function getSerialized(page, filter = undefined) {