FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: test-tls-fast-writing replace common.fixturesDir by bthallion · Pull Request #15808 · nodejs/node · GitHub

/ node Public

test: test-tls-fast-writing replace common.fixturesDir - #15808

Closed
bthallion wants to merge 0 commit into
nodejs:masterfrom
bthallion:master
Closed

test: test-tls-fast-writing replace common.fixturesDir#15808
bthallion wants to merge 0 commit into
nodejs:masterfrom
bthallion:master

Conversation

bthallion commented Oct 6, 2017
edited
Loading

Copy link
Copy Markdown

In test-tls-fast-writing I used the common/fixtures module to reference fixturesDir rather than the common/index module

Checklist
Affected core subsystem(s)

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 6, 2017
Comment thread test/parallel/test-tls-fast-writing.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

common should be the first thing require()'ed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've fixed this

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with the nit addressed.

Comment thread test/parallel/test-tls-fast-writing.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you replace the three readFileSyncs (e.g. fs.readFileSync(${dir}/test_ca.pem) with fixtures.readSync('test_ca.pem')) you can remove the const dir line, and probably the require('fs') as well, although make lint will tell you that.

If you could push a commit to your branch making that change that'd be great!

Docs here: https://github.com/nodejs/node/tree/master/test/common#fixturesreadsyncargs-enc

Trott closed this Oct 15, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 15, 2017
Replace common.fixturesDir with fixtures module usage in
test/parallel/test-tls-fast-writing.js.

PR-URL: nodejs#15808
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Trott commented Oct 15, 2017

Copy link
Copy Markdown
Member

Landed in 2b76b5d.
Thanks for the contribution! 🎉

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
Replace common.fixturesDir with fixtures module usage in
test/parallel/test-tls-fast-writing.js.

PR-URL: nodejs/node#15808
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
Replace common.fixturesDir with fixtures module usage in
test/parallel/test-tls-fast-writing.js.

PR-URL: #15808
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
Replace common.fixturesDir with fixtures module usage in
test/parallel/test-tls-fast-writing.js.

PR-URL: #15808
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Replace common.fixturesDir with fixtures module usage in
test/parallel/test-tls-fast-writing.js.

PR-URL: #15808
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Replace common.fixturesDir with fixtures module usage in
test/parallel/test-tls-fast-writing.js.

PR-URL: #15808
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. 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.

10 participants


Back | FazBrowse Home | New Git URL