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.

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

test: improve assert message in test-dh-regr #15912

Closed

Conversation

Epitrochoid
Copy link

Part of the Node Interactive Code and Learn.

My task suggested improving the assert message in test-dh-regr as it currently doesn't print the failing values. To make the output readable I specified 'base64' as the encoding and print the secrets on test failure.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the crypto Issues and PRs related to the crypto 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
Trott
Trott previously approved these changes Oct 8, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Oct 8, 2017

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.

This actually changes the test somewhat since it is not clear from the test if the same code path are still hit or not. That is probably the case but I would still prefer not to use base64 when calling computeSecret. The return value is a buffer and that could be changed to base64 when printing, instead of changing the arguments.

@Trott Trott dismissed their stale review October 9, 2017 03:21

Agree with @BridgeAR that it would be preferable to not change the test, and that base64 can happen inside the template literal. Not a blocking objection for me, though.

@Epitrochoid Epitrochoid force-pushed the update-assert-messages branch 2 times, most recently from 015035c to 2249287 Compare October 9, 2017 17:56
@Epitrochoid
Copy link
Author

@BridgeAR @Trott great suggestion, changed it to convert to base64 after the secret is computed.

@Trott
Copy link
Member

Trott commented Oct 10, 2017

This looks good, but it results in a lint error. (Run make lint-js on POSIX or vcbuild lint-js on Windows.) Perhaps change the long line to something like this?:

    'Secrets should be equal.\n' +
    `aSecret: ${aSecret.toString('base64')}\n` +
    `bSecret: ${bSecret.toString('base64')}`

@Epitrochoid Epitrochoid force-pushed the update-assert-messages branch from 2249287 to 97dfdb3 Compare October 11, 2017 21:16
@Epitrochoid
Copy link
Author

Good catch, thanks. Fixed the long line, but I wasn't sure if it would be better to have aSecret, bSecret or have them on separate lines, so I left that part as is.

@Trott
Copy link
Member

Trott commented Oct 12, 2017

jasnell pushed a commit that referenced this pull request Oct 13, 2017
PR-URL: #15912
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

Landed in 42dfde8. Thank you!

@jasnell jasnell closed this Oct 13, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#15912
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #15912
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
PR-URL: #15912
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
PR-URL: #15912
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #15912
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants