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

test: refactor test-tls-env-extra-ca #13886

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 23, 2017

  • Use common.mustCall() to guarantee noop function is called
  • Order modules according to test writing guide
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test tls

@Trott Trott added test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Jun 23, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 23, 2017

if (process.env.CHILD) {
const copts = {
port: process.env.PORT,
checkServerIdentity: common.noop,
checkServerIdentity: common.mustCall(),
};
const client = tls.connect(copts, function() {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be 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 for tls.createServer() below.

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau Yes to both. Good catch. Added! Thanks.

const tls = require('tls');

const fork = require('child_process').fork;
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be reordered?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it separate from the rest because of the .fork appended to the end. I think our test writing guidelines might be ambiguous on this situation. ¯\(ツ)

* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide
@Trott Trott force-pushed the refactor-test-tls-env-extra-ca branch from 3714d52 to 7552cc3 Compare June 23, 2017 18:37
@Trott
Copy link
Member Author

Trott commented Jun 24, 2017

@Trott
Copy link
Member Author

Trott commented Jun 26, 2017

Landed in 062c414

@Trott Trott closed this Jun 26, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jun 26, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: nodejs#13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 29, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: #13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@addaleax addaleax mentioned this pull request Jun 29, 2017
addaleax pushed a commit that referenced this pull request Jul 11, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: #13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: #13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: #13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: #13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: #13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
@Trott Trott deleted the refactor-test-tls-env-extra-ca branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants