Implement Puppeteer adapter for Polly that works in web workers in order to test extensions #13870

marekweb · 2020-09-16T18:20:22Z

Add a new PuppeteerAdapter for Polly for intercepting HTTP requests.

Current status: this is a draft PR and includes a temporary test script (run withts-node test-puppeteer-adapter.ts). There are still some errors that happen while intercepting some types of requests.

Problem background

This solves the limitation that the default Puppeteer adapter for Polly does not work to intercept requests inside of web workers.

This limitation is blocking us from writing integration tests for Sourcegraph extensions.

Issues: #12194 #12180
PR which is blocked: #12195

Implementation

The new PuppeteerAdapter uses puppeteer-interceptor which is a wrapper around the Chrome DevTools Protocol.

It uses CDP's "Fetch domain" to intercept requests, which seems to be the best way to do it right now -- however this part of CDP is still flagged as ~~experimental.~~ Actually, I don't see any mention in the docs of it being experimental, so that may have been outdated information.

Alternatives to consider

Because puppeteer-interceptor makes it a bit difficult the follow the flow of the code, and because it's a relatively small wrapper around CDP calls, we could use CDP directly instead and it might result in a cleaner solution. This could be a next step after we get this current solution working properly.

Update and resolution

The above is no longer current. I abandoned the puppeteer-interceptor approach and implemented the alternative instead. See the update comments on this PR for the details.

marekweb · 2020-09-23T01:15:32Z

After some trial and error on this PR, I created CdpAdapter: an adapter that calls the Chrome DevTools Protocol directly (the Fetch domain of CDP) instead of doing it through a library.

The advantages of this new adapter over the earlier PuppeteerAdapter:

it removes the dependency on puppeteer-interceptor
simplified control flow: no control promises involved
it removes a layer of indirection between Polly and CDP, and makes more straightforward to convert between their respective request/response formats

What's left on CdpAdapter:

~~Implement the request pattern matching options. Currently it captures all requests.~~ Decided to exclude this from the scope of the current PR
~~Validate if request recording works correctly~~ New issue created: Implement Polly recording/replaying of requests #14174
Ignore errors from sending CDP requests after the page was closed.
Handle failed requests properly, which means proper conversion between Polly and CDP failed requests.

felixfbecker

Skipped the console.logs and TODOs, this all looks really good!

…remove old polly puppeteer-adapter

marekweb · 2020-09-25T04:14:17Z

The Polly adapter types definition (the Adapter base class that we use) is missing some types (namely the constructor argument and the entire handleRequest method).

I tried to fix the types by adding declarations, or inlining a fixed declaration, but I didn't find a working solution. I spent too much time on the problem already so I kept two @ts-ignore comments.

felixfbecker · 2020-09-25T10:37:34Z

shared/src/testing/integration/polly/CdpAdapter.ts

+     * interception using the CDP "Fetch domain".
+     */
+    public async onConnect(): Promise<void> {
+        console.log('CDP adapter connecting')


Suggested change

console.log('CDP adapter connecting')

felixfbecker · 2020-09-25T10:37:40Z

shared/src/testing/integration/context.ts

@@ -96,7 +96,7 @@ export const createSharedIntegrationTestContext = async <
    directory,
 }: IntegrationTestOptions): Promise<IntegrationTestContext<TGraphQlOperations, TGraphQlOperationNames>> => {
    await driver.newPage()
-    await driver.page.setRequestInterception(true)
+    // await driver.page.setRequestInterception(true)


Suggested change

// await driver.page.setRequestInterception(true)

felixfbecker · 2020-09-25T10:41:18Z

test-puppeteer-adapter.ts

@@ -0,0 +1,71 @@
+import { Polly } from '@pollyjs/core'


Can this file be deleted?

felixfbecker · 2020-09-25T14:47:25Z

Closing this in favor of #12195, since @marekweb merged this branch into it

offirgolan · 2020-09-25T18:09:13Z

shared/src/testing/integration/polly/CdpAdapter.ts

+     * return a Promise of the Response for that request, which will be passed to
+     * request.respond().
+     */
+    public passthroughRequest(pollyRequest: PollyRequest & PollyRequestArguments): Promise<PollyResponse> {


The name of this method is a bit misleading. Its purpose is to create a brand new request given the pollyRequest argument and return the response. This is used internally by both the record and passthrough handlers to get the response data for the given pollyRequest. The reason a duplicate request is necessary is because you don't want to respond to the original request until polly is done with it (i.e. via mutations with client side server events) and also the actual request could have been modified during the request event which means you might be making a completely different request than the original.

Would the current implementation of this be an acceptable simplification if we don't make use of request mutation anywhere (we don't)? Or is there a reason that wouldn't work? Iirc the duplicate requests posed some problems because they can't be 1:1 identical in some regard but I don't fully remember

Would the current implementation of this be an acceptable simplification if we don't make use of request mutation anywhere

I would say most likely not since this is how all adapters are currently expected to behave. I mostly see usages of the request event when wanting to change auth headers or user credentials so I feel like this is pretty important to try incorporate. We could list this as a 3rd party adapter and document this as a caveat until we can get this working properly.

Iirc the duplicate requests posed some problems because they can't be 1:1 identical in some regard but I don't fully remember

The biggest reason we ran into issues was because we were making native browser fetch requests which were an issue when the intercepted request was made via XHR. I don't think you'll run into the same issues here but I'm sure there might be some other complexities as I'm not fully familiar with CDP.

Thanks, I see, I agree that it should be solved before we upstream this. Do you think there would be a problem with using this in our test suite this way though given that we know that we don't change auth headers or credentials etc?

Yeah, i think it should be fine as long as you don't actually fully respond to the original request until respondToRequest is called.

offirgolan · 2020-09-25T18:13:48Z

shared/src/testing/integration/polly/CdpAdapter.ts

+    public async onConnect(): Promise<void> {
+        this.cdpSession = await this.page.target().createCDPSession()
+
+        // TODO: This is where we narrow down the interception with patterns.


I would also add a check to see if you're already intercepting requests on the page.

Fetch example:

this.assert( 'Running concurrent fetch adapters is unsupported, stop any running Polly instances.', !context.fetch[IS_STUBBED] && !context.Request[IS_STUBBED] );

offirgolan · 2020-09-25T18:17:11Z

shared/src/testing/integration/polly/CdpAdapter.ts

+                requestId,
+                responseCode: pollyResponse.statusCode,
+                responseHeaders: headerObjectToHeaderEntries(headers),
+                body: toBase64(body),


Does the body always have to be in base64? That will be unexpected for users that are expecting JSON or text. Can we detect if the body is utf8 via import { isBufferUtf8Representable } from '@pollyjs/utils'?

Loïc Guychard and others added 6 commits September 2, 2020 18:00

use puppeteer-interceptor for puppeteeradapter

20c5579

reinstate requestResourceTypes

e676ea7

Logs

9536319

add doc

c6c2d58

Create test script and fix puppeteer adapter

cfc3dd0

Add working CdpAdapter

dad2d60

marekweb mentioned this pull request Sep 23, 2020

Add integration tests for extension functionality (get Puppeteer to intercept web worker requests) #12194

Closed

felixfbecker reviewed Sep 23, 2020

View reviewed changes

felixfbecker added ops & tools & dev team/web testing Issues that deal with unit tests, integration tests and the testing infrastructure. labels Sep 23, 2020

marekweb added 3 commits September 24, 2020 07:58

Improve CDP request sending and body b64 encoding

4da92e8

Change test script

eb5f84a

Merge branch 'main' into lg/puppeteer-cdp

649de07

marekweb mentioned this pull request Sep 24, 2020

Implement Polly recording/replaying of requests #14174

Closed

marekweb added 3 commits September 24, 2020 11:49

Remove puppeteer-interceptor and patterns

eae5e92

Remove extra console logs

d8f996c

Fix TS errors in adapter, refactor error handling, add doc comments, …

8af2b5e

…remove old polly puppeteer-adapter

marekweb marked this pull request as ready for review September 25, 2020 04:09

marekweb requested a review from tjkandala September 25, 2020 04:09

felixfbecker mentioned this pull request Sep 25, 2020

feat: Simplify PuppeteerAdapter to avoid creating duplicate requests Netflix/pollyjs#348

Closed

9 tasks

felixfbecker reviewed Sep 25, 2020

View reviewed changes

felixfbecker added 6 commits September 25, 2020 14:12

Merge branch 'main' into lg/puppeteer-cdp

ae9eb70

Remove logs

0b13abc

Don't use hardcoded status code

3065002

Add docblock

b71f29d

Correct ID

8677faf

Type adapter options

d22c84d

felixfbecker added 9 commits September 25, 2020 15:39

Remove import

d3f97b9

Correct getter

e13111f

Correct ID

e4a447d

Fix ID

c6fbde7

Remove test script

15a9823

Reference IDs

f8e2e7b

Try to fix flaky test

8157747

Add debug log

f893923

Try $eval

254fd99

felixfbecker closed this Sep 25, 2020

offirgolan reviewed Sep 25, 2020

View reviewed changes

keegancsmith deleted the lg/puppeteer-cdp branch January 27, 2023 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Puppeteer adapter for Polly that works in web workers in order to test extensions #13870

Implement Puppeteer adapter for Polly that works in web workers in order to test extensions #13870

marekweb commented Sep 16, 2020 •

edited

marekweb commented Sep 23, 2020 •

edited

felixfbecker left a comment

marekweb commented Sep 25, 2020 •

edited

felixfbecker Sep 25, 2020

felixfbecker Sep 25, 2020

felixfbecker Sep 25, 2020 •

edited

felixfbecker commented Sep 25, 2020

offirgolan Sep 25, 2020

felixfbecker Sep 25, 2020

offirgolan Sep 25, 2020

felixfbecker Sep 25, 2020

offirgolan Sep 25, 2020

offirgolan Sep 25, 2020

offirgolan Sep 25, 2020

Implement Puppeteer adapter for Polly that works in web workers in order to test extensions #13870

Implement Puppeteer adapter for Polly that works in web workers in order to test extensions #13870

Conversation

marekweb commented Sep 16, 2020 • edited

marekweb commented Sep 23, 2020 • edited

felixfbecker left a comment

Choose a reason for hiding this comment

marekweb commented Sep 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker Sep 25, 2020 • edited

Choose a reason for hiding this comment

felixfbecker commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekweb commented Sep 16, 2020 •

edited

marekweb commented Sep 23, 2020 •

edited

marekweb commented Sep 25, 2020 •

edited

felixfbecker Sep 25, 2020 •

edited