-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Properly add/cleanup process event listeners. #2693
Conversation
This is news to me! |
@sgress454 , you're right, my description was not correct. I've restored the use of .once() .on() to the original use, but kept the cleanup in app.lower(). I've added tests that demonstrates the warning that typicall arises in a test suite (test/integration/lift.lower.test.js) and a unit test that checks that we remove the eventlisteners on lower (test/unit/app.lower.test.js). |
966d5cc
to
8057e1a
Compare
91e6a08
to
ae3c428
Compare
Hm, the tests fail when I run this--it looks like it might not be lowering correctly, because I get a
in the |
@sgress454 It fails in travis as well but not for me locally so I'm a bit puzzelled. Any clue on what kind of environment I need to test with? (I'm running node 0.12 on linux). Also, it looks to me like the travis run fails during lift (not lower). Could it be that a previous test doesn't lower correctly? |
This fails when Node is <0.12 |
Fixes warnings when doing lift/lower multiple times, e.g in before() and after() functions in a test suite. See test/integration/lift.lower.test.js
@sgress454 @washimimizuku thanks y'all @haavardw would you mind rebasing from master and giving this another go? For a while this year, a PR was merged that added something to our npm test script that caused it to fail intermittently for reasons unrelated to the content of the PR (it was checking duplicate code). It would definitely be worth it to give it a fresh run on the latest and see where it stands. |
Works for me now on all Node versions, and passed Travis w/ Node 10. I'd say it's good to go. |
Properly add/cleanup process event listeners.
No longer necessary thanks to #2693
No longer necessary thanks to balderdashy#2693
No longer necessary thanks to balderdashy#2693
Add and cleanup process event listeners properly.
Fixes warnings when doing lift/lower multiple times, e.g in before() and
after() functions in a test suite.
Tests:
test/integration/lift.lower.test.js
test/unit/app.lower.test.js