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

Enhance Cipheriv, Decipheriv coverage #17458

Closed
wants to merge 3 commits into from
Closed

Enhance Cipheriv, Decipheriv coverage #17458

wants to merge 3 commits into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Dec 4, 2017

I added these tests:

Cipheriv
  • Call constructor without new keyword
  • Call constructor with cipher is not string
  • Call constructor with cipher is string and key is not string
  • Call constructor with cipher is strong, key is string and iv is not string
Decipheriv
  • Call constructor without new keyword
  • Call constructor with cipher is not string
  • Call constructor with cipher is string and key is not string
  • Call constructor with cipher is strong, key is string and iv is not string

Current coverage is here: https://coverage.nodejs.org/coverage-06e1b0386196f8f8/root/internal/crypto/cipher.js.html

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@Leko
test: add missing test about crypto.Cipheriv()
378abff 
- Call constructor without new keyword
- Call constructor with cipher is not string
- Call constructor with cipher is string and key is not string
- Call constructor with cipher is strong, key is string and iv is not string
@Leko
test: add missing test about crypto.Decipheriv()
b7dee5e 
- Call constructor without new keyword
- Call constructor with cipher is not string
- Call constructor with cipher is string and key is not string
- Call constructor with cipher is strong, key is string and iv is not string
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 4, 2017
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 4, 2017
@maclover7
Copy link
Contributor

Just a general note, is there any advantage to not using the createDecipheriv/createCipheriv methods like the other tests in the file, for consistency?

This was referenced Dec 4, 2017
Closed
Closed
@Leko
Copy link
Contributor Author

Leko commented Dec 5, 2017

@maclover7 Thank you for your review.
I lost the point of consistency. I thought it would be better to use a factory.
I updated new XXX -> crypto.createXXX.

maclover7
maclover7 approved these changes Dec 5, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

One quick nit then LGTM

test/parallel/test-crypto-cipheriv-decipheriv.js
const key = '123456789012345678901234';
const iv = '12345678';

const instance = Cipheriv('des-ede3-cbc', key, iv);
Copy link
Contributor

Choose a reason for hiding this comment

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

createCipheriv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line cannot use createCipheriv because createCipheriv calls constructor with new keyword always.
I want to test call constructor without new keyword.

@maclover7
Copy link
Contributor

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

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 7, 2017
@apapirovski
Copy link
Member

Landed in 6707903

@apapirovski apapirovski closed this Dec 8, 2017
apapirovski pushed a commit that referenced this pull request Dec 8, 2017
@Leko @apapirovski
test: improve coverage for Cipheriv and Decipheriv
6707903 
PR-URL: #17458
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Leko Leko deleted the enhance_coverage_crypto_cipheriv branch December 8, 2017 15:49
@MylesBorins
Copy link
Contributor

This relies on changes in #16527

Setting as don't land for <=v9.x

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
@Leko Leko mentioned this pull request Dec 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@maclover7 maclover7 maclover7 approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto 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.
10 participants
@Leko @maclover7 @apapirovski @MylesBorins @jasnell @lpinca @BridgeAR @mscdex @addaleax @nodejs-github-bot