Skip to content

Test: use common.fixtures#15965

Closed
obensource wants to merge 4 commits into
nodejs:masterfrom
obensource:bpm/common-fixtures-module-refactor
Closed

Test: use common.fixtures#15965
obensource wants to merge 4 commits into
nodejs:masterfrom
obensource:bpm/common-fixtures-module-refactor

Conversation

@obensource
Copy link
Copy Markdown
Member

@obensource obensource commented Oct 6, 2017

Replaces use of common.fixturesDir with common.fixtures module in test/parallel/test-tls-max-send-fragment.js.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@obensource obensource force-pushed the bpm/common-fixtures-module-refactor branch from e9cd496 to 6a98d26 Compare October 6, 2017 17:53
@mscdex mscdex added the tls Issues and PRs related to the tls 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 7, 2017
View reviewed changes
Copy link
Copy Markdown
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.

There is actually a dedicated function to load the keys directly (readKey). Please use that instead.
@obensource
Copy link
Copy Markdown
Member Author

obensource commented Oct 8, 2017 •
edited
Loading

@BridgeAR done! Note: something that is slightly confusing in this context is that readKey handles both certs and keys, though it makes implicit sense since they exist in /keys. ¯_(ツ)_/¯
@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Oct 8, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10485/
cjihrig
cjihrig reviewed Oct 8, 2017
View reviewed changes
Comment thread test/parallel/test-tls-max-send-fragment.js Outdated
const server = tls.createServer({
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
key: fixtures.readKey('/agent1-key.pem'),
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig Oct 8, 2017

Choose a reason for hiding this comment

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

I think you can drop the leading slashes.
@tniessen
Copy link
Copy Markdown
Member

tniessen commented Oct 9, 2017

@obensource Certificates are essentially keys with some metadata, so it is kind of an abstraction.
@obensource
Copy link
Copy Markdown
Member Author

obensource commented Oct 9, 2017

@tniessen rad, that makes sense. Thanks! 🍻
@Trott
Copy link
Copy Markdown
Member

Trott commented Oct 9, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10521/
@obensource
Copy link
Copy Markdown
Member Author

obensource commented Oct 9, 2017 •
edited by Trott
Loading

@joyeecheung fyi CI 10485 failure here:

479 | sequential/test-async-wrap-getasyncid |  
-- | -- | --
duration_ms0.2severitycrashedstackoh no! exit code: CRASHED |   | duration_ms | 0.2 |   | severity | crashed |   | stack | oh no! exit code: CRASHED
  | duration_ms | 0.2
  | severity | crashed
  | stack | oh no! exit code: CRASHED

Related to reopened issue: #13020

Not something to additionally address in this tls test PR I'd guess–hence approval?

Edited by Trott for formatting.
@BridgeAR BridgeAR self-assigned this Oct 9, 2017
BridgeAR pushed a commit that referenced this pull request Oct 9, 2017
@BridgeAR
test: use common.fixtures in tls test
17857d4 
PR-URL: #15965
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@BridgeAR
Copy link
Copy Markdown
Member

BridgeAR commented Oct 9, 2017

Landed in 17857d4

Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 !
@BridgeAR BridgeAR closed this Oct 9, 2017
@obensource
Copy link
Copy Markdown
Member Author

obensource commented Oct 9, 2017

@BridgeAR Wahoo! Thank you! 🎉🎉🎉

So excited to jump in–thanks for reviewing! 😎
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@MylesBorins
test: use common.fixtures in tls test
329d22f 
PR-URL: #15965
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Oct 11, 2017
Merged
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
@addaleax
test: use common.fixtures in tls test
a15a49d 
PR-URL: nodejs/node#15965
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
@MylesBorins
test: use common.fixtures in tls test
36e7344 
PR-URL: #15965
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@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 common.fixtures in tls test
73c23b7 
PR-URL: #15965
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@MylesBorins
test: use common.fixtures in tls test
e68ef29 
PR-URL: #15965
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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.

9 participants

@obensource @joyeecheung @tniessen @Trott @BridgeAR @cjihrig @mscdex @MylesBorins @nodejs-github-bot