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 of test-tls-over-http-tunnel #18784

Closed

Conversation

juggernaut451
Copy link
Contributor

  • 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

Changes that are done in file are :

  • replaced function with arrow function
  • adding descripiton about test
  • repalced callback with common.mustCall

#14544

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 14, 2018
@@ -24,6 +24,9 @@ const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

// This test ensures that the data throught tls over http tunnel
Copy link
Member

Choose a reason for hiding this comment

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

Typo: throught -> through

Also, probably data received through rather than just data through?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes made. Thank you @Trott for feedback

@jasnell
Copy link
Member

jasnell commented Feb 16, 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 16, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
PR-URL: nodejs#18784
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 16, 2018

Landed in 2960096 🎉

@juggernaut451 I fixed the commit message while landing. Please check our commit guidelines briefly. They are always included as default text when opening a PR. Those checkboxes should actually also be checked accordingly :-)

@BridgeAR BridgeAR closed this Feb 16, 2018
@juggernaut451
Copy link
Contributor Author

juggernaut451 commented Feb 16, 2018

@BridgeAR will read it and will keep check from next time.

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
PR-URL: #18784
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
PR-URL: #18784
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18784
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
PR-URL: #18784
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@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