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.

Sign up for GitHub

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

Jump to bottom

Fix integration test "Stamp Editor Copy/paste from a tab to an other" #18318

Closed
timvandermeij opened this issue Jun 23, 2024 · 2 comments · Fixed by #18320
Closed

Fix integration test "Stamp Editor Copy/paste from a tab to an other" #18318

timvandermeij opened this issue Jun 23, 2024 · 2 comments · Fixed by #18320
Assignees
@timvandermeij
Labels
test

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 23, 2024 •
edited
Loading

The integration test runs currently contain the following traceback in the logs:

Suite error: Stamp Editor Copy/paste from a tab to an other
  Message:
�[31m    ProtocolError: Runtime.callFunctionOn timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.�[0m
  Stack:
        at <instance_members_initializer> (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/common/CallbackRegistry.js:89:14)
        at new Callback (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/common/CallbackRegistry.js:93:16)
        at CallbackRegistry.create (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/common/CallbackRegistry.js:19:26)
        at Connection._rawSend (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/Connection.js:86:26)
        at CdpCDPSession.send (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/CDPSession.js:63:33)
        at #evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/ExecutionContext.js:353:50)
        at ExecutionContext.evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/ExecutionContext.js:268:36)
        at IsolatedWorld.evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/cdp/IsolatedWorld.js:96:30)
        at CdpFrame.evaluate (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/api/Frame.js:335:43)
        at CdpFrame.<anonymous> (file:///home/ubuntu/pdfjs/botio-files-pdfjs/private/ae7a2979558207a/node_modules/puppeteer-core/lib/esm/puppeteer/util/decorators.js:94:27)

I have bisected this to #18313, and only now noticed that this was actually already present in the integration test run logs at http://54.241.84.105:8877/ae7a2979558207a/output.txt and http://54.193.163.58:8877/5ae7c79b8bea53b/output.txt in that PR. However, sadly this didn't actually make the integration tests fail, and this bug is filed separately in #18319.

The problem is that this test opens two pages, whereas the other tests always open just one. The test copies the image from page 1 to page 2 and then stops. This means that at the end of the test page 2 has focus, but in the afterAll handler we close page 1 before page 2, in other words: we attempt to close the page first that doesn't have focus.

Before this PR that wouldn't have been an issue, but now that the L10n logic waits for requestAnimationFrame that hangs page 1's tab because, until there is actually focus, no new request animation frame will come. This in turn causes the test to hang in the afterAll handler until the timeout happens, or if the user manually clicks the first tab in the browser.

One could argue that this is an issue in the PR, but on the other hand it was done like that for good reason and I also don't really see a better way. It seems easier to just change the test, since there is only one that actually suffers from this, to close the pages in reverse order so that we always close the tab with focus first, after which the other tab will automatically get focus and close too.
@timvandermeij timvandermeij self-assigned this Jun 23, 2024
@Snuffleupagus
Copy link
Collaborator

Hopefully Fluent itself can be updated to address what at least I consider a bug, as discussed in #18313 (comment), by changing https://github.com/projectfluent/fluent.js/blob/fee2242049010f314345a7c3b7b91600df3a05fe/fluent-dom/src/dom_localization.js#L171-L172 into e.g.

this.windowElement?.cancelAnimationFrame(this.pendingrAF);
this.windowElement = null;
this.pendingrAF = null;

since that'd allow us to remove our requestAnimationFrame when destroying the l10n-class.

@timvandermeij timvandermeij mentioned this issue Jun 23, 2024
Closed
timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue Jun 23, 2024
@timvandermeij
Fix the "copy/paste from a tab to an other" stamp editor integration …
ddd6b17 
…test

This integration test contains three issues:

- The `page.bringToFront()` calls is not awaited, even though it returns
  a promise (see https://pptr.dev/api/puppeteer.page.bringtofront). Note
  that in other tests we do this correctly already.
- The `page.waitForSelector()` call at the end is unnecessary because
  that exact condition is already checked at the end of the
  `waitForImage` function we call just before this line; see
  https://github.com/mozilla/pdf.js/blob/master/test/integration/stamp_editor_spec.mjs#L74.
- The pages should be closed in reversed order; please refer to the
  description in mozilla#18318 for more details.

Fixes mozilla#18318.
@timvandermeij timvandermeij mentioned this issue Jun 23, 2024
Merged
timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue Jun 23, 2024
@timvandermeij
Fix the "copy/paste from a tab to an other" stamp editor integration …
287fd6a 
…test

This integration test contains three issues:

- The `page.bringToFront()` call is not awaited, even though it returns
  a promise (see https://pptr.dev/api/puppeteer.page.bringtofront). Note
  that in other tests we do this correctly already.
- The `page.waitForSelector()` call at the end is unnecessary because
  that exact condition is already checked at the end of the
  `waitForImage` function we call just before this line; see
  https://github.com/mozilla/pdf.js/blob/master/test/integration/stamp_editor_spec.mjs#L74.
- The pages should be closed in reversed order; please refer to the
  description in mozilla#18318 for more details.

Fixes mozilla#18318.
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Jun 23, 2024 •
edited
Loading

Hopefully Fluent itself can be updated to address what at least I consider a bug

Yes, I agree. For now we can work around this (fortunately in an easy manner; see the PR above) until Fluent implements this functionality, but it's indeed extra reason to implement this upstream in Fluent and not the projects using it.

@timvandermeij timvandermeij mentioned this issue Jun 23, 2024
Merged
@timvandermeij timvandermeij mentioned this issue Jun 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timvandermeij timvandermeij

Labels
test
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix the "copy/paste from a tab to an other" stamp editor integration test timvandermeij/pdf.js
2 participants
@timvandermeij @Snuffleupagus