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

Replaced https-socket-options test using fixtures #15882

Closed
wants to merge 4 commits into from
Closed

Replaced https-socket-options test using fixtures #15882

wants to merge 4 commits into from

Conversation

lamchakchan
Copy link

NINA 2017 Code and Learn session for first time contributors. This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

Checklist
  • 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: replaced fs.readSync with fixtures.readSync
06cf9f2 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
pawelgolda
pawelgolda reviewed Oct 6, 2017
View reviewed changes
test/parallel/test-https-socket-options.js Outdated
@@ -30,8 +31,8 @@ const fs = require('fs');
const http = require('http');

const options = {
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fixtures.readSync(`/keys/agent1-key.pem`),
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be readKey method instead of readSync

test: optimizing the use of the fixtures library
949ae1b 
changing calls from fixtures.readSync to fixtures.readKey where
applicable.
@lamchakchan
Copy link
Author

Thanks for catch that @pawelgolda

test: fixed typo
1b5a460 
fixed typo with pem key`
@mscdex mscdex added the https Issues or PRs related to the https 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
lpinca
lpinca approved these changes Oct 10, 2017
View reviewed changes
test/parallel/test-https-socket-options.js Outdated
@@ -21,6 +21,7 @@

'use strict';
const common = require('../common');
const fixtures = 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: can you please move this after the common.hasCrypto check?

@gireeshpunathil
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Oct 13, 2017

CI is showing green, but no tests were actually run. Something is not working on CI. Will have to run again after it's fixed.

@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

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

joyeecheung
joyeecheung previously requested changes Oct 14, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Linter complained:

not ok 2 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-https-socket-options.js
  ---
  message: '''fs'' is assigned a value but never used.'
  severity: error
  data:
    line: 30
    column: 7
    ruleId: no-unused-vars
  ...

can you remove const fs = require('fs'); at line 30?

@jasnell jasnell dismissed joyeecheung’s stale review October 15, 2017 22:12

Fixed nits in a new commit

jasnell pushed a commit that referenced this pull request Oct 15, 2017
@jasnell
test: replaced fs.readSync with fixtures.readSync
bb9ed92 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

PR-URL: #15882
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 15, 2017

Landed in bb9ed92

@jasnell jasnell closed this Oct 15, 2017
targos pushed a commit that referenced this pull request Oct 18, 2017
@targos
test: replaced fs.readSync with fixtures.readSync
b49df0b 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

PR-URL: #15882
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
@addaleax
test: replaced fs.readSync with fixtures.readSync
805ba42 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

PR-URL: nodejs/node#15882
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@MylesBorins
test: replaced fs.readSync with fixtures.readSync
04ea78d 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

PR-URL: #15882
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@MylesBorins
test: replaced fs.readSync with fixtures.readSync
55aef4f 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

PR-URL: #15882
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@MylesBorins
test: replaced fs.readSync with fixtures.readSync
2ab826c 
NINA 2017 Code and Learn session for first time contributors.  This
touched test/parallel/test-https-socket-options.js to replace
common.fixturesDir

PR-URL: #15882
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pawelgolda pawelgolda pawelgolda left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@joyeecheung joyeecheung joyeecheung left review comments

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. https Issues or PRs related to the https 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.