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

fix MaxListenersExceededWarning #910

Merged
merged 2 commits into from Sep 30, 2017

Conversation

Projects
None yet
2 participants
@mizdra
Contributor

mizdra commented Sep 29, 2017

A EventListener is not removed when Protocol error (Page.navigate) is catched. This causes MaxListenersExceededWarning.

@aslushnikov

Awesome, thank you for the PR.

lgtm % test comment

const closedPage = await browser.newPage();
await closedPage.close();
process.once('warning', warning => expect(warning).toBe(null));

This comment has been minimized.

@aslushnikov

aslushnikov Sep 29, 2017

Contributor

this event listener should be removed after the test.

note: You can just copy the previous test and use 'asdf' instead of EMPTY_PAGE as url:

    it('should not leak listeners during bad navigation', SX(async function() {
      let warning = null;
      const warningHandler = w => warning = w;
      process.on('warning', warningHandler);
      for (let i = 0; i < 20; ++i)
        await page.goto('asdf').catch(e => {/* swallow navigation error */});
      process.removeListener('warning', warningHandler);
      expect(warning).toBe(null);
    }));

This comment has been minimized.

@mizdra

mizdra Sep 30, 2017

Contributor

Oh, I fixed it.

Thank you 😃

improve test
- add removeListener
- use 'asdf' instead of EMPTY_PAGE

@aslushnikov aslushnikov merged commit 215b349 into GoogleChrome:master Sep 30, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment