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

test: verify listener leak is only emitted once #12502

Merged
merged 1 commit into from Apr 21, 2017

Conversation

@cjihrig
Copy link
Contributor

commented Apr 19, 2017

When a possible listener leak is detected, a warning is emitted. This commit updates an existing test to verify that the warning is only emitted once.

In conjunction with #12501, should bring coverage of lib/events.js up to 100%.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@mscdex mscdex added the events label Apr 19, 2017

@lpinca
lpinca approved these changes Apr 19, 2017
@Fishrock123
Copy link
Member

left a comment

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

The only CI failures were unrelated.

test: verify listener leak is only emitted once
When a possible listener leak is detected, a warning is
emitted. This commit updates an existing test to verify that the
warning is only emitted once.

PR-URL: #12502
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

@cjihrig cjihrig force-pushed the cjihrig:no-warn branch to 5e095f6 Apr 21, 2017

@cjihrig cjihrig merged commit 5e095f6 into nodejs:master Apr 21, 2017

@cjihrig cjihrig deleted the cjihrig:no-warn branch Apr 21, 2017

@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Should be backported with #12043

gibfahn added a commit that referenced this pull request Jun 20, 2017
test: verify listener leak is only emitted once
When a possible listener leak is detected, a warning is
emitted. This commit updates an existing test to verify that the
warning is only emitted once.

PR-URL: #12502
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Landed this on v6.x-staging as e08113e but had to modify as common.noop doesn't seem to have been backported.

@cjihrig can you just quickly confirm that this is correct?

@gibfahn gibfahn added land-on-v6.x and removed lts-watch-v6.x labels Jun 20, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2017

Looks correct to me. Thanks.

MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: verify listener leak is only emitted once
When a possible listener leak is detected, a warning is
emitted. This commit updates an existing test to verify that the
warning is only emitted once.

PR-URL: #12502
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.