fix: disable restored mocks by KuznetsovRoman · Pull Request #10413 · webdriverio/webdriverio

KuznetsovRoman · 2023-05-18T14:54:49Z

Proposed changes

So Mock has clear and restore, but it does not stop requests/responses from being processed just like active mocks.

request / match / continue / ... events will still be emitted, and now there is no way to prevent it.

Example:

    it('some test', async () => {
        const mock = await browser.mock('**', {
            headers: {
                'content-type': 'text/html'
            }
        })
        mock.on('request', () => console.log('opening wdio.io page'))
        await browser.url('https://webdriver.io')
        // logs 'opening wdio.io page' once
    })
    
    it('some other test', async () => {
        const mock = await browser.mock('**', {
            headers: {
                'content-type': 'text/html'
            }
        })
        mock.on('request', () => console.log('opening wdio.io page'))
        await browser.url('https://webdriver.io')
        // logs 'opening wdio.io page' twice
    })

It affects memory usage.

Example:

it('test 0', async () => {
    const mock = await browser.mock('**');
    mock.restore();
    setInterval(() => {
        console.log(mock.calls.length);
    }, 5000);
})
Array(100).fill(0).map((_, i) => {
    it(`test ${i + 1}`, async () => {
        await browser.url(`https://youtube.com`);
    })
})

produces this log (tail), where the number comes from console.log(mock.calls.length):

3352
[16:48:06 GMT+3] ✓ test 94 [chrome:66454ccd-cb0d-45f2-9dcd-f30994a3e0f6] - 2331ms
[16:48:09 GMT+3] ✓ test 95 [chrome:66454ccd-cb0d-45f2-9dcd-f30994a3e0f6] - 2354ms
3421
[16:48:11 GMT+3] ✓ test 96 [chrome:66454ccd-cb0d-45f2-9dcd-f30994a3e0f6] - 2456ms
[16:48:14 GMT+3] ✓ test 97 [chrome:66454ccd-cb0d-45f2-9dcd-f30994a3e0f6] - 2796ms
3491
[16:48:17 GMT+3] ✓ test 98 [chrome:66454ccd-cb0d-45f2-9dcd-f30994a3e0f6] - 2592ms
[16:48:19 GMT+3] ✓ test 99 [chrome:66454ccd-cb0d-45f2-9dcd-f30994a3e0f6] - 2456ms
3555
[16:48:24 GMT+3] ✓ test 100 [chrome:c8ffdf93-165f-4377-9f09-16a9de8e9247] - 3174ms

so, it looks like those calls are being saved, and if each test has a mock, then 100-th test will be stored 100 times.

Example:

Array(100).fill(0).map((_, i) => {
    it(`test ${i + 1}`, async () => {
        const mock = await browser.mock('**');
        await browser.url(`https://youtube.com`);
        mock.restore(); // replace with mock.remove to fix exponential memory leak
    })
})

uses 3.5GB ram:

It affects performance (not much, really, but still): with just mock.restore at the end of test, 100-th test takes 1171 ms, and with mock.remove - 810ms (on simple test: mock = browser.mock('**'); browser.url('localhost:3000'); mock.remove, where localhost:3000 serves react app template, generated with create-react-app). mock.remove also disables fetch domain, if there are no other mocks. I assume, fetch domain has some effect on performance.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
I have added proper type definitions for new commands (if appropriate)

Reviewers: @webdriverio/project-committers

packages/webdriverio/src/utils/interception/index.ts

KuznetsovRoman · 2023-05-18T15:00:43Z

packages/webdriverio/src/commands/browser/mock.ts

    const networkInterception = new NetworkInterception(url, filterOptions, browser)
    SESSION_MOCKS[handle].add(networkInterception as Interception)

+    networkInterception.remove = () => {


couldn't figure out how to name it properly, because there are clear and restore

I thought about replacing clear/restore with this, but that would be a breaking change.

And yes, defining method like this does not look nice, but i coulnd't figure out a better alternative, because:

you need networkInterception in order to remove it from set

you need SESSION_MOCKS, to know, where to delete from

you need handle, to know where to exactly delete from

you need this client: CDPSession to disable fetch domain,

and accessing all of those from DevtoolsInterception hurts.

Let me know, if you could think of any better way to do it.

I agree, this is not the right place to define this command in and we should find ways to put it in NetworkInterception.

you need networkInterception in order to remove it from set

If you define this method in the NetworkInterception class it is accessible through this.

you need SESSION_MOCKS, to know, where to delete from

SESSION_MOCKS then needs to be defined somewhere where mock.ts and the file where NetworkInterception is defined can import it from without having circular dependencies.

you need handle, to know where to exactly delete from

make remove async and get it via const handle = await browser.getWindowHandle()

you need this client: CDPSession to disable fetch domain,

You have access to the browser variable so you can get that variable too.

The method mockRestore is meant to do what you want to do with remove .. can we just make sure the restore method ensures that nor memory is hogged?

This is an issue that should be addressed but I am not happy with the current approach

That's why i converted it to draft: i found a better way to do it

You have access to the browser variable so you can get that variable too

i tried, and that's what i discovered: you can get puppeteer through browser, but not client: CDPSession, because it would be different client, and we need original one. Anyway, it's manageable

The method mockRestore is meant to do what you want to do with remove .. can we just make sure the restore method ensures that nor memory is hogged?

There is also a moment to consider. There are two options:

restore also sends Fetch.disable, if there are no other mocks (if restore means "i dont need this anymore". In this case we would have to enable fetch domain again after some respond / abort)

restore does not disable fetch domain, even if there are no other mocks. (if you plan to use respond / abort after it was restored. In this case we could just ensure this mock presents in SESSION_MOCKS)

First option also makes restore async for devtools and breaks existing "respond / abort after restore" cases, so i think about implementing second approach, but at first i need to clarify: what happens if you try to "respond / abort after restore" with saucelabs? Will it work just like mock wasn't restored?

We have designed the API to be as close as possible to the Jest Mock API so I would say the first should be more appropriate. I feel honestly less worried about the Sauce Labs integration. I was working there at a time I build this and don't have any usage numbers. At the end of the day this API should support what will be supported in WebDriver Bidi because Sauce eventually has to support that.

christian-bromann

Thanks for taking a stab at this. This is an issue that should be addressed but I am not happy with the current approach.

christian-bromann · 2023-05-18T17:00:20Z

packages/webdriverio/src/commands/browser/mock.ts

    const networkInterception = new NetworkInterception(url, filterOptions, browser)
    SESSION_MOCKS[handle].add(networkInterception as Interception)

+    networkInterception.remove = () => {


I agree, this is not the right place to define this command in and we should find ways to put it in NetworkInterception.

you need networkInterception in order to remove it from set

If you define this method in the NetworkInterception class it is accessible through this.

you need SESSION_MOCKS, to know, where to delete from

SESSION_MOCKS then needs to be defined somewhere where mock.ts and the file where NetworkInterception is defined can import it from without having circular dependencies.

you need handle, to know where to exactly delete from

make remove async and get it via const handle = await browser.getWindowHandle()

you need this client: CDPSession to disable fetch domain,

You have access to the browser variable so you can get that variable too.

christian-bromann · 2023-05-18T17:05:00Z

packages/webdriverio/src/commands/browser/mock.ts

    const networkInterception = new NetworkInterception(url, filterOptions, browser)
    SESSION_MOCKS[handle].add(networkInterception as Interception)

+    networkInterception.remove = () => {


The method mockRestore is meant to do what you want to do with remove .. can we just make sure the restore method ensures that nor memory is hogged?

KuznetsovRoman

Now mock.restore() acts like reversed browser.mock().

KuznetsovRoman · 2023-05-19T08:37:45Z

packages/webdriverio/src/commands/browser/mock.ts

+import type { CDPSession } from 'puppeteer-core/lib/esm/puppeteer/common/Connection.js'

 export const SESSION_MOCKS: Record<string, Set<Interception>> = {}
+export const CDP_SESSIONS: Record<string, CDPSession> = {}


const client = await page.target().createCDPSession() creates new CDPSession, but we need the original one to disable fetch domain. Storing map<handle, CDPSession> allows us to restore mocks in any order

KuznetsovRoman · 2023-05-19T08:39:04Z

packages/webdriverio/src/utils/interception/devtools.ts

     * removes any mocked return values or implementations.
     */
-    restore () {
+    async restore (sessionMocks = SESSION_MOCKS, cdpSessions = CDP_SESSIONS) {


this is for unit tests

KuznetsovRoman · 2023-05-19T08:42:47Z

packages/webdriverio/src/utils/interception/devtools.ts

+    private ensureNotRestored() {
+        if (this.restored) {
+            throw new Error('This can\'t be done on restored mock')
+        }
+    }


If somebody has tests, where abort / respond is called after restore, currently it would work just like creating the new one with mocked response.

After this commit, abort / respond after restore just won't do anything, so i added this check to let people know, that you can't do that now.

Also, should i include BREAKING CHANGE in commit message footer? That could be considered as breaking change.

KuznetsovRoman · 2023-05-19T11:08:39Z

Also added a commit to handle error responses.

Currently, event is considered a request event, if it has no status code, but some non-request events also dont have it.

Event example:

{
  requestId: 'interception-job-3.0',
  request: {
    url: 'https://foo-bar-baz-qux.com/image.jpeg', // when host is not resolved
    method: 'GET',
    headers: { ... },
    initialPriority: 'Low',
    referrerPolicy: 'strict-origin-when-cross-origin'
  },
  frameId: '1A8B140A1A0109D1F6C38483096CB34F',
  resourceType: 'Image',
  responseErrorReason: 'NameNotResolved',
  networkId: '7803.5'
}

I fixed it, so those events would be handled as not-requests, and we would see them.

For example:

mock.on("match", match => {
    console.log(match.statusCode); // will prints "0", if there is no such host. Currently, we just wont have that event.
})

0 is also a falsy value, so we could use !match.statusCode

christian-bromann

Could it be that what you are trying to do is more close towards what a restoreAllMocks would do?

christian-bromann · 2023-05-19T14:57:21Z

packages/webdriverio/src/utils/interception/devtools.ts

+        }
+
+        log.trace(`Disabling fetch domain for ${handle}`)
+        return cdpSessions[handle].send('Fetch.disable')


This would restore mocking for all mocks right? Can you verify that you can create two mocks within the same session and have only one restored?

this would restore mocking for all mocks right?

we have SESSION_MOCKS, which is map<handle, mock[]>
All mocks of one handle use the same CDP SESSION, and share the same fetch domain

In this implementation i added CDP_SESSIONS, which stores handle -> CDPSession

When we remove last mock of some handle, we dont need to intercept requests for this CDPSession, and it actually disables fetch domain for this handle only.

If we will create two mocks within the same session and then restore just one of them, condition on L#238 will be true, and we will return from this method on L#239б so other mocks of this handle would still act as unrestored. I also included tests for this case:

disables fetch domain after restore, if there are no other mocks

does not disable fetch domain after restore, if there are other mocks

Makes sense, so only if all mocks of a specific window are restored, the Fetch.disable command is being called. We should update mockRestoreAll.ts to add an await to the restore call.

Yup. Fixed in ee4f276

Also changed type here
and used await instead of return here, because Promise<Promise<void>> and Promise<void> are not the same thing

christian-bromann · 2023-05-19T18:11:57Z

@KuznetsovRoman it seems like some unit tests are failing 🤔

KuznetsovRoman · 2023-05-19T18:21:14Z

@KuznetsovRoman it seems like some unit tests are failing thinking

Yes, it's the one, checking webdriver's mock "restore" method return value. I changed "return" to "await" there, so it now returns undefined, instead of "{}". Do we need this return value, or why do we even need this check in the first place?

christian-bromann · 2023-05-19T18:36:33Z

Do we need this return value, or why do we even need this check in the first place?

We don't need the return value, just have the expected value be undefined then. The check is good so we make sure whatever we call works and doesn't throw, due to non existence for example

KuznetsovRoman · 2023-05-19T18:40:46Z

Should be fixed

christian-bromann

Thank you, LGTM 👍

KuznetsovRoman commented May 18, 2023

View reviewed changes

KuznetsovRoman marked this pull request as draft May 18, 2023 15:33

christian-bromann reviewed May 18, 2023

View reviewed changes

KuznetsovRoman force-pushed the main branch from 8143c77 to e13384e Compare May 19, 2023 08:34

KuznetsovRoman changed the title ~~feat: mock.remove & removeAllMocks~~ fix: disable restored mocks May 19, 2023

KuznetsovRoman force-pushed the main branch from e13384e to 589cbba Compare May 19, 2023 08:48

KuznetsovRoman commented May 19, 2023

View reviewed changes

KuznetsovRoman marked this pull request as ready for review May 19, 2023 09:10

christian-bromann reviewed May 19, 2023

View reviewed changes

KuznetsovRoman force-pushed the main branch from 7b61fc7 to 48ab331 Compare May 19, 2023 16:00

KuznetsovRoman added 2 commits May 19, 2023 21:39

fix: disable mocks after restore

1366084

fix: mock response error

c51baac

KuznetsovRoman force-pushed the main branch from 48ab331 to c51baac Compare May 19, 2023 18:40

christian-bromann approved these changes May 19, 2023

View reviewed changes

christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label May 19, 2023

christian-bromann merged commit d2906da into webdriverio:main May 19, 2023

Uh oh!

Conversation

KuznetsovRoman commented May 18, 2023

Proposed changes

Types of changes

Checklist

Reviewers: @webdriverio/project-committers

Uh oh!

Uh oh!

KuznetsovRoman May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KuznetsovRoman May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christian-bromann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KuznetsovRoman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KuznetsovRoman commented May 19, 2023

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KuznetsovRoman May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christian-bromann commented May 19, 2023

Uh oh!

KuznetsovRoman commented May 19, 2023

Uh oh!

christian-bromann commented May 19, 2023

Uh oh!

KuznetsovRoman commented May 19, 2023

Uh oh!

christian-bromann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KuznetsovRoman May 18, 2023 •

edited

Loading

KuznetsovRoman May 18, 2023 •

edited

Loading

christian-bromann left a comment •

edited

Loading

KuznetsovRoman May 19, 2023 •

edited

Loading