test: refactor test-beforeexit-event #10121

radelmann · 2016-12-05T06:31:13Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

replaced var with const or let.
removed all console.log() statements.
removed deaths and revivals vars.
wrapped beforexit listener callbacks with
common.mustCall().
removed exit event listener.

mscdex · 2016-12-05T07:04:19Z

test/parallel/test-beforeexit-event.js


-function tryTimer() {
+const tryTimer = () => {


Why is this necessary?

mscdex · 2016-12-05T07:04:28Z

test/parallel/test-beforeexit-event.js


-function tryListen() {
+const tryListen = () => {


Same here, why is this necessary?

mscdex · 2016-12-05T07:05:19Z

test/parallel/test-beforeexit-event.js

-}
+};
+
+process.on('beforeExit', () => { deaths++; });


I think we should just get rid of the revivals and deaths variables and just use common.mustCall() with the expected number of calls.

Fishrock123 · 2016-12-05T14:34:27Z

test/parallel/test-beforeexit-event.js

+}
+
+function beforeProcess() {
+  console.log('before process');


maybe better as before listener?

I thought we were trying to remove all logging in tests unless required by the test?

Maybe but it just gets swallowed anyways?

The console.log are definitely not required though so ¯\_(ツ)_/¯

Fishrock123 · 2016-12-05T14:36:45Z

test/parallel/test-beforeexit-event.js

-  assert.equal(3, revivals);
+process.on('exit', () => {
+  common.mustCall(beforeExit, 4);
+  common.mustCall(beforeProcess, 3);


That's not how this works. :)

You will need to wrap the callback that you want to call before using it (as in, the variables at lines 9-15 already need the wrappers). Then the exit event listener can be removed.

radelmann · 2016-12-06T18:43:36Z

@Fishrock123 requested changes have been completed, any additional feedback would be much appreciated. Thanks:-)

mscdex · 2016-12-06T20:11:38Z

test/parallel/test-beforeexit-event.js


-process.on('beforeExit', function() { deaths++; });
+const beforeExit = common.mustCall(() => {


This can just be:

process.on('beforeExit', common.mustCall(() => {}), 4);

mscdex · 2016-12-06T20:13:05Z

test/parallel/test-beforeexit-event.js

      this.close();
+    })
+    .on('error', (e) => {


This 'error' handler can be removed since node will already throw if there are no 'error' handlers.

mscdex · 2016-12-06T20:16:17Z

test/parallel/test-beforeexit-event.js

-  setImmediate(function() {
-    revivals++;
+  setImmediate(() => {
+    beforeListener();


What I had in mind instead of this is to wrap the enclosing function in common.mustCall(), for example:

setImmediate(common.mustCall(() => { process.once('beforeExit', common.mustCall(tryTimer)); }));

mscdex · 2016-12-06T20:17:27Z

test/parallel/test-beforeexit-event.js

-  setTimeout(function() {
-    console.log('timeout cb, do another once beforeExit');
-    revivals++;
+  setTimeout(() => {


Similarly, this would instead be:

setTimeout(common.mustCall(() => { process.once('beforeExit', common.mustCall(tryListen)); }));

mscdex · 2016-12-06T20:18:17Z

test/parallel/test-beforeexit-event.js

    process.once('beforeExit', tryListen);
  }, 1);
 }

 function tryListen() {
-  console.log('create a server');
  net.createServer()
    .listen(0)
    .on('listening', function() {


and this one would just be

.on('listening', common.mustCall(() => { this.close(); }));

mscdex · 2016-12-06T20:18:45Z

test/parallel/test-beforeexit-event.js

+  return;
+}, 3);
+
+process.on('beforeExit', beforeExit);

 process.once('beforeExit', tryImmediate);


This needs to be:

process.once('beforeExit', common.mustCall(tryImmediate));

Fishrock123 · 2016-12-06T21:31:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5273/

Fishrock123

just a little nit

Fishrock123 · 2016-12-06T21:31:42Z

test/parallel/test-beforeexit-event.js

+}, 4);
+
+const beforeListener = common.mustCall(() => {
+  return;


These can probably just be () => {}

mscdex · 2016-12-06T21:54:59Z

test/parallel/test-beforeexit-event.js


-process.on('beforeExit', function() { deaths++; });
+const beforeExit = common.mustCall(() => {}, 4);


Now that I think about it, I think we can actually get rid of this separate check entirely by just adding a

process.on('beforeExit', common.mustCall(() => {}));

after the this.close() in tryListen().

Fishrock123

CI again: https://ci.nodejs.org/job/node-test-pull-request/5277/

mscdex · 2016-12-07T03:06:15Z

LGTM

mhdawson

LGTM

mhdawson · 2016-12-07T15:49:14Z

CI clean except for failure on smartos which seems unrelated to this test. Opened #10166 to track that issue separately.

mhdawson · 2016-12-07T16:04:14Z

@radelmann, one last thing before we land. We need your full name "Rob Adelmann" as the author for the commit. I could fix that as part of landing but probably better if you do this in your env and re-push. You should be able to change with: git commit --amend --author="Rob Adelmann [email protected]" and if you can also update your git config so that future commits also have your full name that would be good.

addaleax · 2016-12-07T17:03:02Z

We need your full name "Rob Adelmann" as the author for the commit.

Just for clarity, feel free to use the name and email address you would like your commits to be credited to (or indicate that just “adelmann” is actually exactly what you prefer).

- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener.

radelmann · 2016-12-07T17:31:01Z

@mhdawson & @addaleax, commit author amended. Let me know if you need anything else, thanks.

addaleax · 2016-12-07T21:30:58Z

Landed in 8960383, thanks for the contribution!

- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: nodejs#10121 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener. PR-URL: #10121 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 5, 2016

radelmann force-pushed the test-beforeexit-event-refactor branch from cda4900 to 48691c1 Compare December 5, 2016 06:32

mscdex added the process Issues and PRs related to the process subsystem. label Dec 5, 2016

mscdex reviewed Dec 5, 2016

View reviewed changes

test/parallel/test-beforeexit-event.js

function tryTimer() {

const tryTimer = () => {

Copy link

Contributor

mscdex Dec 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

mscdex reviewed Dec 5, 2016

View reviewed changes

test/parallel/test-beforeexit-event.js

function tryListen() {

const tryListen = () => {

Copy link

Contributor

mscdex Dec 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why is this necessary?

mscdex reviewed Dec 5, 2016

View reviewed changes

radelmann force-pushed the test-beforeexit-event-refactor branch 2 times, most recently from faa9ace to 0eddb09 Compare December 5, 2016 08:03

Fishrock123 suggested changes Dec 5, 2016

View reviewed changes

radelmann force-pushed the test-beforeexit-event-refactor branch 5 times, most recently from cba9aff to 3e3fe42 Compare December 5, 2016 21:33

mscdex reviewed Dec 6, 2016

View reviewed changes

radelmann force-pushed the test-beforeexit-event-refactor branch from 29b7e97 to 8f8c63e Compare December 6, 2016 20:35

Fishrock123 suggested changes Dec 6, 2016

View reviewed changes

mscdex reviewed Dec 6, 2016

View reviewed changes

radelmann force-pushed the test-beforeexit-event-refactor branch from 8f8c63e to 5a9a898 Compare December 6, 2016 22:05

Fishrock123 approved these changes Dec 7, 2016

View reviewed changes

jasnell approved these changes Dec 7, 2016

View reviewed changes

mhdawson approved these changes Dec 7, 2016

View reviewed changes

test: refactor test-beforeexit-event

4b254d8

- replaced var with const/let. - removed all console.log() statements. - removed deaths and revivals vars. - wrapped beforexit listener callbacks with common.mustCall(). - removed exit event listener.

radelmann force-pushed the test-beforeexit-event-refactor branch from 5a9a898 to 4b254d8 Compare December 7, 2016 17:26

addaleax closed this Dec 7, 2016

addaleax added the land-on-v6.x label Dec 8, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

MylesBorins added the land-on-v4.x label Dec 20, 2016

This was referenced Dec 21, 2016

V6.9.3 proposal #10394

Merged

V4.7.1 proposal #10395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactor test-beforeexit-event #10121

test: refactor test-beforeexit-event #10121

radelmann commented Dec 5, 2016 •

edited

Loading

mscdex Dec 5, 2016

mscdex Dec 5, 2016

mscdex Dec 5, 2016

Fishrock123 Dec 5, 2016

mscdex Dec 5, 2016

Fishrock123 Dec 5, 2016 •

edited

Loading

Fishrock123 Dec 5, 2016

radelmann commented Dec 6, 2016

mscdex Dec 6, 2016

mscdex Dec 6, 2016

mscdex Dec 6, 2016

mscdex Dec 6, 2016

mscdex Dec 6, 2016

mscdex Dec 6, 2016

Fishrock123 commented Dec 6, 2016

Fishrock123 left a comment

Fishrock123 Dec 6, 2016

mscdex Dec 6, 2016 •

edited

Loading

Fishrock123 left a comment

mscdex commented Dec 7, 2016

mhdawson left a comment

mhdawson commented Dec 7, 2016

mhdawson commented Dec 7, 2016

addaleax commented Dec 7, 2016

radelmann commented Dec 7, 2016

addaleax commented Dec 7, 2016 •

edited

Loading


		process.on('beforeExit', function() { deaths++; });
		const beforeExit = common.mustCall(() => {

test: refactor test-beforeexit-event #10121

test: refactor test-beforeexit-event #10121

Conversation

radelmann commented Dec 5, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

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

Fishrock123 Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radelmann commented Dec 6, 2016

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

Fishrock123 commented Dec 6, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

mscdex commented Dec 7, 2016

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Dec 7, 2016

mhdawson commented Dec 7, 2016

addaleax commented Dec 7, 2016

radelmann commented Dec 7, 2016

addaleax commented Dec 7, 2016 • edited Loading

radelmann commented Dec 5, 2016 •

edited

Loading

Fishrock123 Dec 5, 2016 •

edited

Loading

mscdex Dec 6, 2016 •

edited

Loading

addaleax commented Dec 7, 2016 •

edited

Loading