Refactor test http exceptions #18506

Bamieh · 2018-02-01T13:39:06Z

Reopens: #17199
I had a busy schedule so i was not able to rebase and the PR was closed. The PR was approved but needed a rebase with master due to some conflicts in which i resolved in this PR.

Refactored the test case test-http-exceptions to use countdown, as per issue [Tracking issue] Converting tests to use Countdown #17169
Added common.mustCall to the countdown callback, since the user will always want this function to be called, and it must fail if the user fails to decrease the counter to 0.
Removed explicit common.mustCall functions around the Countdown callbacks in all the tests.
Add test case for countdown to test that a mustCall is in place.
Updated the common.md docs to inform the countdown users that the test will fail if the countdown callback was not called.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

apapirovski

LGTM with some nits

apapirovski · 2018-02-01T13:42:00Z

test/common/README.md

@@ -380,7 +380,8 @@ Synchronous version of `spawnPwd`.
 The `Countdown` module provides a simple countdown mechanism for tests that
 require a particular action to be taken after a given number of completed
 tasks (for instance, shutting down an HTTP server after a specific number of
-requests).
+requests). Note that _the Countdown will fail the test if the remainder
+did not reach 0_.


The italics aren't necessary here, IMO.

I would go so far and remove the Note that as well.

apapirovski · 2018-02-01T13:44:54Z

test/parallel/test-http-exceptions.js

+
+process.on('uncaughtException',
+           common.mustCall(onUncaughtException, NUMBER_OF_EXCEPTIONS)
+);


Can this be brought up to the previous line? The dangling ); is awkward.

sadly it cant be moved up since it will break the linter due to reaching the max limit of 80 characters in one line

It's only 70 chars as far as I can tell?

apapirovski · 2018-02-01T13:46:38Z

test/parallel/test-http-exceptions.js

 const http = require('http');
+const NUMBER_OF_EXCEPTIONS = 4;
+const countdown = new Countdown(NUMBER_OF_EXCEPTIONS, () => {
+  process.exit(0);


Does server.close() not do the trick here?

it does not, probably because of the intended exception throwing intentionally_not_defined. it keeps res from being closed with .end();.

BridgeAR

LGTM with the comments addressed.

BridgeAR · 2018-02-01T14:05:45Z

test/common/README.md

@@ -380,7 +380,8 @@ Synchronous version of `spawnPwd`.
 The `Countdown` module provides a simple countdown mechanism for tests that
 require a particular action to be taken after a given number of completed
 tasks (for instance, shutting down an HTTP server after a specific number of
-requests).
+requests). Note that _the Countdown will fail the test if the remainder
+did not reach 0_.


I would go so far and remove the Note that as well.

Bamieh · 2018-02-01T15:00:33Z

@BridgeAR @apapirovski updated as per feedback

BridgeAR · 2018-02-06T21:58:53Z

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

Bamieh · 2018-02-07T09:51:32Z

@BridgeAR can you explain why is this failing? how can i solve it? seems like a rebase issue or something, should i rebase commits into one commit?

BridgeAR · 2018-02-07T13:15:10Z

@Bamieh there is a merge commit in here. Please rebase and force push with lease. The CI is not able to rebase this otherwise.

Bamieh · 2018-02-07T13:59:53Z

@BridgeAR Thanks! I believe i did what is needed. can you re-run the CI?

BridgeAR · 2018-02-07T14:04:26Z

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

apapirovski · 2018-02-08T17:00:51Z

@Bamieh it looks like this has a typo in one of the test files. Could you fix and then I'll start another CI?

Bamieh · 2018-02-09T11:25:49Z

@apapirovski im getting this error:

Error: Cannot parse file .remarkrc

This has nothing to do with my change, and i am upto date with master. Any idea? this PR has been rough for some reason :/

Also the tests were passing before I merge master a few days ago. Running make test-parallel succeeds normally (which is where i did my code changes)

$ [02:41|% 100|+ 1817|-   0]: Done

remove typo

BridgeAR · 2018-02-09T12:38:49Z

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

Bamieh · 2018-02-11T10:15:23Z

warning: There are too many unreachable loose objects; run 'git prune' to remove them.

Should i run git prune to fix the failing ones? its only giving

Checking connectivity: 377643, done.

Bamieh · 2018-02-12T16:58:45Z

@BridgeAR
node-test-commit is not working, i tried digging into the logs and this is what i found, not sure what is the root cause of this though.

TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set '*.tap' is empty

in https://ci.nodejs.org/job/node-test-commit-linux/nodes=fedora24/16264/console

Is this something caused by me? should i rebase again or something?

BridgeAR · 2018-02-12T17:09:05Z

@Bamieh the current failures are infrastructure related and have nothing to do with this PR. So you do not have to worry about anything :-)

BridgeAR · 2018-02-12T17:12:26Z

test/parallel/test-http-exceptions.js

+}
+
+process.on('uncaughtException',
+           common.mustCall(onUncaughtException, NUMBER_OF_EXCEPTIONS));


Nit, the common.mustCall is obsolete here, because the test will not execute countdown.dec() in case it would not be called and would then fail because the wrapper around the countdown would fail.

This adds a implicit common.mustCall to the callback provided to the countdown. PR-URL: nodejs#18506 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

BridgeAR · 2018-02-16T17:40:03Z

Landed in 941bc93 🎉

I addressed the nit while landing.

This adds a implicit common.mustCall to the callback provided to the countdown. PR-URL: #18506 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

This adds a implicit common.mustCall to the callback provided to the countdown. PR-URL: nodejs#18506 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

This adds a implicit common.mustCall to the callback provided to the countdown. PR-URL: #18506 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 1, 2018

Bamieh mentioned this pull request Feb 1, 2018

refactor test-http-exceptions file to use countdown #17199

Closed

4 tasks

apapirovski approved these changes Feb 1, 2018

View reviewed changes

BridgeAR approved these changes Feb 1, 2018

View reviewed changes

lpinca approved these changes Feb 1, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2018

Bamieh added 7 commits February 7, 2018 15:53

Fixes: nodejs#17169 (test-http-exceptions)

f802a56

mustCall inside countdown

4cd8fbc

Add test case for countdown

4286f71

Update countdown docs to hint the change

1eb069c

Update README.md

7ad8ce7

Update README.md

6f125e1

update readme

0060979

Bamieh force-pushed the refactor-test-http-exceptions branch from 6abfde3 to c5b6e69 Compare February 7, 2018 13:58

apapirovski removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2018

up up up one line

6840cf3

remove typo

Bamieh force-pushed the refactor-test-http-exceptions branch from c5b6e69 to 6840cf3 Compare February 9, 2018 11:29

jasnell approved these changes Feb 9, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018

BridgeAR reviewed Feb 12, 2018

View reviewed changes

BridgeAR closed this Feb 16, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

MylesBorins added land-on-v8.x and removed dont-land-on-v8.x labels Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor test http exceptions #18506

Refactor test http exceptions #18506

Bamieh commented Feb 1, 2018 •

edited

Loading

apapirovski left a comment

apapirovski Feb 1, 2018

BridgeAR Feb 1, 2018

apapirovski Feb 1, 2018

Bamieh Feb 1, 2018

apapirovski Feb 1, 2018

apapirovski Feb 1, 2018

Bamieh Feb 1, 2018 •

edited

Loading

BridgeAR left a comment

BridgeAR Feb 1, 2018

Bamieh commented Feb 1, 2018 •

edited

Loading

BridgeAR commented Feb 6, 2018

Bamieh commented Feb 7, 2018

BridgeAR commented Feb 7, 2018

Bamieh commented Feb 7, 2018

BridgeAR commented Feb 7, 2018

apapirovski commented Feb 8, 2018

Bamieh commented Feb 9, 2018 •

edited

Loading

BridgeAR commented Feb 9, 2018

Bamieh commented Feb 11, 2018 •

edited

Loading

Bamieh commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

BridgeAR Feb 12, 2018

BridgeAR commented Feb 16, 2018

Refactor test http exceptions #18506

Refactor test http exceptions #18506

Conversation

Bamieh commented Feb 1, 2018 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bamieh Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bamieh commented Feb 1, 2018 • edited Loading

BridgeAR commented Feb 6, 2018

Bamieh commented Feb 7, 2018

BridgeAR commented Feb 7, 2018

Bamieh commented Feb 7, 2018

BridgeAR commented Feb 7, 2018

apapirovski commented Feb 8, 2018

Bamieh commented Feb 9, 2018 • edited Loading

BridgeAR commented Feb 9, 2018

Bamieh commented Feb 11, 2018 • edited Loading

Bamieh commented Feb 12, 2018

BridgeAR commented Feb 12, 2018

Choose a reason for hiding this comment

BridgeAR commented Feb 16, 2018

Bamieh commented Feb 1, 2018 •

edited

Loading

Bamieh Feb 1, 2018 •

edited

Loading

Bamieh commented Feb 1, 2018 •

edited

Loading

Bamieh commented Feb 9, 2018 •

edited

Loading

Bamieh commented Feb 11, 2018 •

edited

Loading