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

Refactor test http exceptions #18506

Closed
wants to merge 8 commits into from

Conversation

Bamieh
Copy link
Contributor

@Bamieh Bamieh commented Feb 1, 2018

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

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

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM with some nits

@@ -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_.
Copy link
Member

Choose a reason for hiding this comment

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

The italics aren't necessary here, IMO.

Copy link
Member

Choose a reason for hiding this comment

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

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


process.on('uncaughtException',
common.mustCall(onUncaughtException, NUMBER_OF_EXCEPTIONS)
);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Bamieh Bamieh Feb 1, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM with the comments addressed.

@@ -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_.
Copy link
Member

Choose a reason for hiding this comment

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

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

@Bamieh
Copy link
Contributor Author

Bamieh commented Feb 1, 2018

@BridgeAR @apapirovski updated as per feedback

@BridgeAR 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
@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

@Bamieh
Copy link
Contributor Author

Bamieh commented Feb 7, 2018

@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
Copy link
Member

BridgeAR commented Feb 7, 2018

@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 Bamieh force-pushed the refactor-test-http-exceptions branch from 6abfde3 to c5b6e69 Compare February 7, 2018 13:58
@Bamieh
Copy link
Contributor Author

Bamieh commented Feb 7, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

@apapirovski
Copy link
Member

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

@apapirovski 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
@Bamieh
Copy link
Contributor Author

Bamieh commented Feb 9, 2018

@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
@Bamieh Bamieh force-pushed the refactor-test-http-exceptions branch from c5b6e69 to 6840cf3 Compare February 9, 2018 11:29
@BridgeAR
Copy link
Member

BridgeAR commented Feb 9, 2018

@BridgeAR 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
@Bamieh
Copy link
Contributor Author

Bamieh commented Feb 11, 2018

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
Copy link
Contributor Author

Bamieh commented Feb 12, 2018

@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
Copy link
Member

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

}

process.on('uncaughtException',
common.mustCall(onUncaughtException, NUMBER_OF_EXCEPTIONS));
Copy link
Member

Choose a reason for hiding this comment

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

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.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: nodejs#18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

Landed in 941bc93 🎉

I addressed the nit while landing.

@BridgeAR BridgeAR closed this Feb 16, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: #18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: #18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: nodejs#18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: #18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: #18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: #18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
This adds a implicit common.mustCall to the callback provided to
the countdown.

PR-URL: #18506
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants