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 file to use countdown #17199

Closed
wants to merge 6 commits into from

Conversation

Bamieh
Copy link
Contributor

@Bamieh Bamieh commented Nov 21, 2017

  • 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

@mscdex mscdex added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Nov 21, 2017
const http = require('http');
const NUMBER_OF_EXCEPTIONS = 4;
const countdown = new Countdown(NUMBER_OF_EXCEPTIONS, () => process.exit(0));
Copy link
Member

@joyeecheung joyeecheung Nov 21, 2017

Choose a reason for hiding this comment

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

This should be wrapped in common.mustCall, also should the listener of process.on('uncaughtException') and server.listen

This kind of reminds me: should we wrap the cb in common.mustCall in the countdown module by default? @jasnell

Copy link
Member

Choose a reason for hiding this comment

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

should we wrap the cb in common.mustCall in the countdown module by default?

Yeah, I've been thinking the same thing. It would be worthwhile I think

Copy link
Member

@joyeecheung joyeecheung Nov 21, 2017

Choose a reason for hiding this comment

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

Come to think of it, maybe this test does not need the countdown module and can just wrap the process.on('uncaughtException') listener in common.mustCall(..., NUMBER_OF_EXCEPTIONS), because every expected action is the same. The countdown module seems to be more suitable for actions that vary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joyeecheung unless i am overlooking something; I didn't wrap it in a mustCall since not invoking the callback will result in a timeout and hence a failure in the test anyways, but adding it does not hurt as well.

Copy link
Member

@Trott Trott Nov 22, 2017

Choose a reason for hiding this comment

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

If you do go with this (and I'm with @joyeecheung that this maybe doesn't need Countdown): Can you please wrap process.exit(0) in { and }?

Without { and }, then the result of process.exit(0) is the return value of the function. That function shouldn't have a return value.

Copy link
Contributor Author

@Bamieh Bamieh Nov 22, 2017

Choose a reason for hiding this comment

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

@Trott process.exit() terminates the process synchronously, so the return value will not get passed back from the invocation, so it does not really matter if you pass an explicit return or not. I'll do it anyways for convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding wrapping the countdown with a mustCall, i think its a good idea to decide soon before contributors start implementing more refactors

Copy link
Member

Choose a reason for hiding this comment

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

I think in general, use common.mustCall if the code calling countdown.dec() is always the same, and use countdown if the caller would be different. That way the code is cleaner. If we are trying to make the code cleaner by using countdown, then I see no reason not to make it even cleaner with common.mustCall

Copy link
Member

Choose a reason for hiding this comment

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

Also...can we implement common.mustCall on top of countdown?

Copy link
Contributor Author

@Bamieh Bamieh Nov 22, 2017

Choose a reason for hiding this comment

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

@joyeecheung i have implemented mustCall on top of countdown, but im not sure i fully understand your suggestion about replacing countdown with mustCall

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

however how are we going to tell the code to process.exit(0)? i cant see that happening without a counter

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM

@maclover7
Copy link
Contributor

maclover7 commented Dec 4, 2017

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.

I think this should be server.close instead? (of course this line would need to be placed after the server creation)

Copy link
Contributor Author

@Bamieh Bamieh Dec 14, 2017

Choose a reason for hiding this comment

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

@joyeecheung calling server.close() does not close the node process for some reason. resulting in a --- TIMEOUT --- in the tests. These are just refactors, and the initial code also calls process.exit. What do you suggest here?

@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Dec 7, 2017
@maclover7
Copy link
Contributor

ping @Bamieh

@Bamieh
Copy link
Contributor Author

Bamieh commented Dec 14, 2017

pong @maclover7

@BridgeAR
Copy link
Member

@Bamieh this needs a rebase.

@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Jan 19, 2018
@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Jan 20, 2018
@BridgeAR
Copy link
Member

Closing due to no further progress. @Bamieh please feel free to reopen if you would like to continue working on it (it only needs a rebase).

@BridgeAR BridgeAR closed this Jan 31, 2018
@Bamieh Bamieh mentioned this pull request Feb 1, 2018
4 tasks
@Bamieh
Copy link
Contributor Author

Bamieh commented Feb 1, 2018

@BridgeAR Sorry i had a busy schedule so i was not able to follow up on this. I opened a new PR for this with the code up-to date with master: #18506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants