Skip to content

test: fix crypto test case to use correct encoding - #17956

Closed
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:fix-authenticated-encoding-test
Closed

test: fix crypto test case to use correct encoding#17956
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:fix-authenticated-encoding-test

Conversation

@tniessen

@tniessentniessen commented Jan 2, 2018

Copy link
Copy Markdown
Member

The callback would have errored out anyway due to the incorrect encoding, and that error is not the point of this test case.

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)

@tniessen
test: fix crypto test case to use correct encoding
f741905
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jan 2, 2018
@tniessentniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 2, 2018
@cjihrig

cjihrig commented Jan 2, 2018

Copy link
Copy Markdown
Contributor

The callback would have errored out anyway due to the incorrect encoding, and that error is not the point of this test case.

Can the error message check be improved so that the test only passes with the correct error?

@tniessen

tniessen commented Jan 2, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

The callback would have errored out anyway due to the incorrect encoding, and that error is not the point of this test case.

Can the error message check be improved so that the test only passes with the correct error?

It already does not pass, but there is no reason to intentionally use an incorrect encoding here. If we really want to pass an invalid encoding, we should at least add a comment. To me, it was not obvious why the test failed with the message [ERR_ASSERTION]: Cannot change encoding when authentication passed.

@tniessen

tniessen commented Jan 2, 2018

Copy link
Copy Markdown
MemberAuthor

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

Trott
Trott reviewed Jan 3, 2018
View reviewed changes
Comment threadtest/parallel/test-crypto-authenticated.js Outdated
} else {
// assert that final throws if input data could not be verified!
assert.throws(function() { decrypt.final('ascii'); }, errMessages.auth);
assert.throws(() => decrypt.final('hex'), errMessages.auth);

@TrottTrottJan 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Micro-nit: I'd prefer we keep braces around this arrow function because it makes it clear that we're not intending to return a value here. Totally ignore me if you have strong opinions to the contrary.

@tniessentniessenJan 3, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done, thank you.

@tniessen

tniessen commented Jan 4, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 5, 2018
@BridgeAR

BridgeAR commented Jan 5, 2018

Copy link
Copy Markdown
Member

Landed in 5160dd0

@BridgeARBridgeAR closed this Jan 5, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 5, 2018
@tniessen@BridgeAR
test: fix crypto test case to use correct encoding
5160dd0
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: nodejs#17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
b875c7b
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
bc40390
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
591dd4e
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Jan 10, 2018
Merged
@TimothyGuTimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
9e9de05
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
8b3c233
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
f0060ba
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
b8c5dd0
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@tniessen@MylesBorins
test: fix crypto test case to use correct encoding
9e76028
The callback would have errored out anyway due to the incorrect
encoding, and that error is not the point of this test case.
PR-URL: #17956
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott left review comments
@jasnelljasnelljasnell approved these changes
@cjihrigcjihrigcjihrig approved these changes
@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

fast-trackPRs that do not need to wait for 48 hours to land.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tniessen@cjihrig@BridgeAR@jasnell@Trott@MylesBorins@TimothyGu@nodejs-github-bot