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

Upgrade Puppeteer to the latest version as the page.off() clean-up function isn't working in the current version #6433

Open
hussain-t opened this issue Jan 19, 2023 · 4 comments
Assignees
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Infrastructure Engineering infrastructure & tooling

Comments

@hussain-t
Copy link
Collaborator

hussain-t commented Jan 19, 2023

Feature Description

The clean-up function page.off("request", requestHandler) in the useRequestInterception utility function is not working in the current version of the puppeteer. This causes test failures when we invoke the clean-up function, which was returned from the useRequestInterception function. The bug was reported in the Puppeteer repository and was fixed in the latest version.

We should upgrade to the latest version of Puppeteer, in which the above bug was fixed, and there are no breaking changes to our existing tests.

Due to the above issue, we introduced a workaround to intercept the &amp_validate requests in #5460.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The latest puppeteer version should be installed and configured in the project.
  • The page.off("request", requestHandler) clean-up function should properly remove request listeners from a page using the latest version of the puppeteer.
  • The upgrade should not break any existing functionality in the existing tests.
  • The changes introduced in E2E tests take much longer on newer versions of WP #5460 should be removed and replaced with the following:
    • It should disable request interception using the clean-up function returned from the useRequestInterception before calling the activateAMPWithMode function.
    • It should re-enable request interception.
    • The activateAMPWithMode function should be modified to intercept the &amp_validate requests.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

@hussain-t hussain-t changed the title Upgrade Puppeteer to the latest version as the page.off('request', requestHandler) clean-up function isn't working in the current version Upgrade Puppeteer to the latest version as the page.off() clean-up function isn't working in the current version Jan 19, 2023
@hussain-t hussain-t self-assigned this Jan 19, 2023
@hussain-t hussain-t added Type: Infrastructure Engineering infrastructure & tooling QA: Eng Requires specialized QA by an engineer labels Jan 19, 2023
@hussain-t hussain-t assigned aaemnnosttv and unassigned hussain-t Jan 19, 2023
@bethanylang
Copy link
Collaborator

@hussain-t Can you please add a priority label to this one? Thank you!

@hussain-t hussain-t added the P1 Medium priority label Jan 23, 2023
@hussain-t
Copy link
Collaborator Author

Added. Thanks @bethanylang

@aaemnnosttv
Copy link
Collaborator

@hussain-t a few things to revise here:

  • The latest puppeteer version should be installed and configured in the project.

The important thing about this issue is that we resolve the problem with page.off – upgrading puppeteer is really more of an implementation detail, but we wouldn't want to consider this done if somehow this behavior was broken again in the latest version :) Similarly, the fix that you've referenced has been merged for several releases now, so there doesn't seem to be a reason why it would need to be the latest version. See puppeteer/puppeteer@d0cb943

Secondarily, it's worth noting that we have both puppeteer and puppeteer-core as dependencies (I don't recall why). Another reason to not make the AC specific to puppeteer as we may need to upgrade both.

Would you please revise the AC to be a little more specific as to what we want to get out of this issue? If we already know more/less how to accomplish that, this can be populated in the IB, we don't need to leave that part empty but of course not worth spending too much time there until AC are solidified 👍

@aaemnnosttv aaemnnosttv assigned hussain-t and unassigned aaemnnosttv Jan 24, 2023
@ivonac4
Copy link
Collaborator

ivonac4 commented Jan 8, 2024

@hussain-t Are you still planning to work on this soon? If not, could you please unassign yourself so someone else can pick it up? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

4 participants