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: change to common fixtures module in test file #15886

Closed

bobclewell wants to merge 2 commits into nodejs:master from bobclewell:feature/replace-fixturesDir-with-fixtures-module-in-http-set-timeout-server

Contributor

bobclewell commented Oct 6, 2017 •

edited

Loading

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

Checklist

make -j4 test (UNIX)
commit message follows commit guidelines

Affected core subsystem(s)

test


          test: change to common fixtures module in test file

a0bc9b2

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

nodejs-github-bot added the test label

mscdex added the https label

Trott added the code-and-learn label

BridgeAR reviewed

View reviewed changes

test/parallel/test-https-set-timeout-server.js Outdated

    
                key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),

                cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)

                key: fs.readFileSync(`${fixtures.fixturesDir}/keys/agent1-key.pem`),

                cert: fs.readFileSync(`${fixtures.fixturesDir}/keys/agent1-cert.pem`)

Member

BridgeAR Oct 9, 2017

There is a dedicated fixtures.readKey function that should be used here. In that case the line is much shorter and you only have to provide the file name.

Contributor Author

bobclewell Oct 9, 2017

@BridgeAR, so for example?

key: fixtures.readKey(`agent1-key.pem`),

lpinca reviewed

View reviewed changes

test/parallel/test-https-set-timeout-server.js Outdated

    
            @@ -21,6 +21,7 @@
          
              'use strict';

              const common = require('../common');

              const fixtures = require('../common/fixtures');

Member

lpinca Oct 10, 2017

Nit: can you please move this after the common.hasCrypto check?

Contributor Author

bobclewell Oct 10, 2017

Done.

test/parallel/test-https-set-timeout-server.js Outdated

    
            @@ -34,8 +35,8 @@ const tls = require('tls');
          
              const tests = [];

              const serverOptions = {

                key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),

                cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)

                key: fs.readFileSync(`${fixtures.readKey('agent1-key.pem')}`),

Member

lpinca Oct 10, 2017

This line can simply be key: fixtures.readKey('agent1-key.pem').

Contributor Author

bobclewell Oct 10, 2017

👍

test/parallel/test-https-set-timeout-server.js Outdated

    
                key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),

                cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)

                key: fs.readFileSync(`${fixtures.readKey('agent1-key.pem')}`),

                cert: fs.readFileSync(`${fixtures.readKey('agent1-cert.pem')}`)

Member

lpinca Oct 10, 2017

Ditto.

Contributor Author

bobclewell Oct 10, 2017

Ditto.

bobclewell force-pushed the feature/replace-fixturesDir-with-fixtures-module-in-http-set-timeout-server branch from f574cd4 to 12da58b Compare

October 10, 2017 13:51

lpinca reviewed

View reviewed changes

test/parallel/test-https-set-timeout-server.js Outdated

    
            @@ -26,6 +26,7 @@ if (!common.hasCrypto)
          
                common.skip('missing crypto');

              const assert = require('assert');

              const fixtures = require('../common/fixtures');

              const fs = require('fs');

Member

lpinca Oct 10, 2017

One last nit: this is no longer needed, so if you can please remove it. Thanks.

Contributor Author

bobclewell Oct 10, 2017

👍

Member

lpinca Oct 10, 2017

Something went wrong with last change as you removed fixtures instead of fs :)

Contributor Author

bobclewell Oct 10, 2017

Oops. Fixed now. 😄
(fingers-crossed)

bobclewell force-pushed the feature/replace-fixturesDir-with-fixtures-module-in-http-set-timeout-server branch from 12da58b to f5572dd Compare

October 10, 2017 14:04


          test: Updated test to use readKey rather than fixturesDir

e1089e2

bobclewell force-pushed the feature/replace-fixturesDir-with-fixtures-module-in-http-set-timeout-server branch from f5572dd to e1089e2 Compare

October 10, 2017 14:20

lpinca approved these changes

View reviewed changes

gireeshpunathil approved these changes

View reviewed changes

Member

gireeshpunathil commented Oct 12, 2017

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

jasnell approved these changes

View reviewed changes

jasnell pushed a commit that referenced this pull request


          test: use fixtures in test-https-set-timeout-server

0e1455b

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

PR-URL: #15886
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Member

jasnell commented Oct 13, 2017

Landed in 0e1455b

jasnell closed this

addaleax pushed a commit to ayojs/ayo that referenced this pull request


          test: use fixtures in test-https-set-timeout-server

239d29f

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

PR-URL: nodejs/node#15886
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request


          test: use fixtures in test-https-set-timeout-server

db92df9

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

PR-URL: #15886
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added the land-on-v6.x label

MylesBorins pushed a commit that referenced this pull request


          test: use fixtures in test-https-set-timeout-server

726c5c4

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

PR-URL: #15886
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request

v6.12.1 proposal #17180

Merged

MylesBorins pushed a commit that referenced this pull request


          test: use fixtures in test-https-set-timeout-server

c60683c

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

PR-URL: #15886
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          test: use fixtures in test-https-set-timeout-server

6b8830c

Task to replace the common.fixturesDir with the usage
of the common.fixtures module. At Node.js Interactive.
First PR to Node.js. Yay!

PR-URL: #15886
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-and-learn https test