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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 0 commits into from
Closed

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

wants to merge 0 commits into from

Conversation

bthallion
Copy link

@bthallion bthallion commented Oct 6, 2017 •
edited
Loading

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 6, 2017
cjihrig
cjihrig reviewed Oct 7, 2017
View reviewed changes
test/parallel/test-tls-fast-writing.js Outdated
@@ -20,6 +20,7 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
const fixtures = require('../common/fixtures');
const common = require('../common');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed this

@Trott 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
benjamingr approved these changes Oct 7, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr 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 nit addressed.

gibfahn
gibfahn reviewed Oct 9, 2017
View reviewed changes
test/parallel/test-tls-fast-writing.js Outdated
const assert = require('assert');
const tls = require('tls');
const fs = require('fs');

const dir = common.fixturesDir;
const dir = fixtures.fixturesDir;
const options = { key: fs.readFileSync(`${dir}/test_key.pem`),
cert: fs.readFileSync(`${dir}/test_cert.pem`),
ca: [ fs.readFileSync(`${dir}/test_ca.pem`) ] };
Copy link
Member

Choose a reason for hiding this comment

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

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 Trott closed this Oct 15, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 15, 2017
@Trott
test: use fixtures module
2b76b5d 
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
Copy link
Member

Trott commented Oct 15, 2017

Landed in 2b76b5d.
Thanks for the contribution! 🎉

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@addaleax
test: use fixtures module
d0b540f 
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
@targos
test: use fixtures module
5629351 
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
@MylesBorins
test: use fixtures module
3f3f457 
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 MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@MylesBorins
test: use fixtures module
1b7d019 
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
@MylesBorins
test: use fixtures module
fafbbb6 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn left review comments

@cjihrig cjihrig cjihrig left review comments

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@rmg rmg rmg approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@bthallion @Trott @rmg @benjamingr @lpinca @cjihrig @gibfahn @mscdex @MylesBorins @nodejs-github-bot