Skip to content

test: replace common.fixturesDir with common.fixtures#15895

Closed
tkg214 wants to merge 1 commit intonodejs:masterfrom
tkg214:patch-test-first-commit
Closed

test: replace common.fixturesDir with common.fixtures#15895
tkg214 wants to merge 1 commit intonodejs:masterfrom
tkg214:patch-test-first-commit

Conversation

@tkg214
Copy link

@tkg214 tkg214 commented Oct 6, 2017 •
edited
Loading

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@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
BridgeAR
BridgeAR reviewed Oct 9, 2017
View reviewed changes
test/parallel/test-https-pfx.js Outdated
Copy link
Member

@BridgeAR BridgeAR Oct 9, 2017

Choose a reason for hiding this comment

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

It seems like the task description was probably not clear enough. I am not sure if this would actually pass or not but the idea was to require the ../common/fixtures and to use the path function from that module instead of using the more verbose version here.

Copy link
Member

@jasnell jasnell Oct 10, 2017

Choose a reason for hiding this comment

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

@tkg214 thank you very much for participating in the code-and-learn. As @BridgeAR indicates, there are some issues with the changes. This should be:

const pfx = fixtures.readSync('test_cert.pfx');

BridgeAR
BridgeAR previously requested changes Oct 9, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Just marking this so it does not get landed until this is fixed.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 18, 2017

Ping @tkg214

@apapirovski
Copy link
Contributor

apapirovski commented Oct 25, 2017

Hi @tkg214, would you like to follow up on the requested changes so this can land? Let me know if any clarifications are needed. Thanks for helping us improve Node!

@Trott Trott force-pushed the patch-test-first-commit branch from e55f865 to 40b8259 Compare October 28, 2017 02:04
@Trott
Copy link
Member

Trott commented Oct 28, 2017

I went ahead and fixed this up per @jasnell's suggestion (and the original intention of the task). Hope that's OK.

This should be good to land. PTAL @BridgeAR

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

joyeecheung
joyeecheung approved these changes Oct 28, 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.

LGTM, CI failures look unrelated

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 31, 2017

ping @BridgeAR again to have another look ( one of 3 remaining code-and-learn PRs!)

@Trott Trott dismissed BridgeAR’s stale review November 1, 2017 03:53

Requested changes have been made.

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 1, 2017
@t-k-g @Trott
test: use fixtures module in test-https-pfx
c52fe67 
PR-URL: nodejs#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 1, 2017

Landed in c52fe67.

Thanks for the contribution! 🎉

@Trott Trott closed this Nov 1, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@t-k-g @Qard
test: use fixtures module in test-https-pfx
83a0540 
PR-URL: nodejs/node#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@t-k-g @Qard
test: use fixtures module in test-https-pfx
1f19d4d 
PR-URL: nodejs/node#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@t-k-g @cjihrig
test: use fixtures module in test-https-pfx
f9b2099 
PR-URL: nodejs#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
@t-k-g @MylesBorins
test: use fixtures module in test-https-pfx
9416dab 
PR-URL: #15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@t-k-g @gibfahn
test: use fixtures module in test-https-pfx
2f66faf 
PR-URL: #15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@t-k-g @addaleax
test: use fixtures module in test-https-pfx
f8f2ebc 
PR-URL: nodejs/node#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments

@watilde watilde watilde approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@BridgeAR BridgeAR BridgeAR 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.