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: replace common.fixturesDir with the fixtures module #16027

Closed
wants to merge 1 commit into from
Closed

test: replace common.fixturesDir with the fixtures module #16027

wants to merge 1 commit into from

Conversation

weiplanet
Copy link
Contributor

@weiplanet weiplanet commented Oct 6, 2017 •
edited by BridgeAR
Loading

Replace common.fixturesDir with use of the common.fixtures module

Checklist
Subsystem

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
mhdawson
mhdawson approved these changes Oct 6, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
benjamingr
benjamingr reviewed Oct 7, 2017
View reviewed changes
test/parallel/test-http-default-port.js
@@ -31,7 +32,7 @@ const assert = require('assert');
const hostExpect = 'localhost';
const fs = require('fs');
const path = require('path');
const fixtures = path.join(common.fixturesDir, 'keys');
const fixtures = path.join(fixturesModule.fixturesDir, 'keys');
Copy link
Member

Choose a reason for hiding this comment

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

This can be a fixures.path('keys')

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better change would be to update the fs.readFileSync() calls a few lines later to use the fixture keys call.

Copy link
Member

Choose a reason for hiding this comment

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

For instance...

const options = {
  key: fixtures.readSync('agent1-key.pem'),
  cert: fixtures.readSync('agent1-cert.pem')
};

@refack refack self-assigned this Oct 10, 2017
@joyeecheung
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10622/

lpinca
lpinca approved these changes Oct 14, 2017
View reviewed changes
test/parallel/test-http-default-port.js
@@ -21,6 +21,7 @@

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

Choose a reason for hiding this comment

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

Nit: this could/should be moved after the common.hasCrypto check.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 14, 2017 •
edited
Loading

Landed in 1a9f48a with #16027 (review) and #16027 (review) fixed. Thank you for the contribution!

joyeecheung pushed a commit that referenced this pull request Oct 14, 2017
@weiplanet @joyeecheung
test: replace fixturesDir with the fixtures module
1a9f48a 
PR-URL: #16027
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@weiplanet @addaleax
test: replace fixturesDir with the fixtures module
d6b06ec 
PR-URL: nodejs/node#16027
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@weiplanet @targos
test: replace fixturesDir with the fixtures module
8452af2 
PR-URL: #16027
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@weiplanet @MylesBorins
test: replace fixturesDir with the fixtures module
81bb560 
PR-URL: #16027
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@weiplanet @MylesBorins
test: replace fixturesDir with the fixtures module
69882ad 
PR-URL: #16027
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@weiplanet @MylesBorins
test: replace fixturesDir with the fixtures module
a28d666 
PR-URL: #16027
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@jasnell jasnell jasnell left review comments

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@hiroppy hiroppy hiroppy approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr 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. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.