test: cleaning up test/parallel/test-crypto-binary-default.js by wuweiweiwu · Pull Request #19054 · nodejs/node

wuweiweiwu · 2018-02-28T08:29:57Z

Moving assertion error message to comment on top of test.
Scoping tests for better readability and debugging.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

watilde · 2018-02-28T12:56:41Z

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

wuweiweiwu · 2018-02-28T17:42:59Z

@watilde It seemed that my commit messages were failing the jenkins build. I have updated my commit messages!

watilde · 2018-02-28T20:15:21Z

@wuweiweiwu Thanks for the update! I will trigger CI again.

watilde · 2018-02-28T20:15:27Z

new ci: https://ci.nodejs.org/job/node-test-pull-request/13411/

wuweiweiwu · 2018-02-28T22:03:09Z

@watilde I am not sure why node-test-commit is failing. I ran core-validate-commit and the only thing failing is commit must have at least 1 reviewer. Do I add the reviewers on the commit after they have approved? In that case I have updated my commit messages again :)

The default message will be printed if the assertion fires. PR-URL: nodejs#19054 Reviewed-By: James M Snell <jasnell@gmail.com>

Puts related variables and tests in the same scope. PR-URL: nodejs#19054 Reviewed-By: James M Snell <jasnell@gmail.com>

watilde · 2018-03-01T12:46:28Z

@wuweiweiwu Thanks for checking it! I just read the ci logs(16560, 2562, 16741, 16051) and the errors seem not related to this patch :) Let's leave this for one more day to get more review.

watilde · 2018-03-01T12:46:48Z

new ci for the last updates: https://ci.nodejs.org/job/node-test-pull-request/13421/

BridgeAR

LGTM, just one nit about something that was there before.

BridgeAR · 2018-03-01T21:12:13Z

test/parallel/test-crypto-binary-default.js

+            'e2adebeb10a298dd'
+      }
+    },



This line seems obsolete and should probably be removed.

Can you briefly explain why the line is obsolete?

These are entries in an array and all other entries are separated without extra line break.

@BridgeAR Fixed :)

apapirovski

I'm really not clear why block scoping in this test is worth the massive diff, given how little else is changing... If something was, I wouldn't even be able to find it with 800 insertions + deletions... Does anyone that reviewed this actually claim to have scanned line by line? There isn't even any variable re-use, race conditions, or anything else here that block scoping helps us resolve.

I hate to be a downer but I'm really -1 on these type of changes.

BridgeAR · 2018-03-02T13:00:24Z

@apapirovski I personally feel similar about this as you but it felt like it does not hurt either and that is why I signed it. It is possible to review this by using the split view. In that case it is much easier to scan line by line.

Trott · 2018-03-02T19:35:03Z

@apapirovski @BridgeAR Add ?w=1 to the diff URL in GitHub to ignore whitespace-only changes to make a diff like this much easier to read: https://github.com/nodejs/node/pull/19054/files?w=1

FWIW, I think adding block scope is higher value than a lot of the changes we otherwise seem to routinely approve, like changing functions to arrow functions.

apapirovski · 2018-03-02T19:40:44Z

@Trott thanks! Is that somewhere in the GitHub interface or just a well-known trick?

Trott · 2018-03-02T21:49:58Z

@Trott thanks! Is that somewhere in the GitHub interface or just a well-known trick?

AFAIK it is not in the interface nor particularly well-known, but wow is it useful IMO.

removing extra line break

apapirovski · 2018-03-04T11:38:36Z

Landed in 1ebd966

The default message will be printed if the assertion fires. Use block scope for related variables and tests. PR-URL: #19054 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

apapirovski · 2018-03-04T11:39:31Z

@wuweiweiwu Congrats on landing your first commit in Node.js core! 🎉 Hope there are many more to come :)

The default message will be printed if the assertion fires. Use block scope for related variables and tests. PR-URL: nodejs#19054 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The default message will be printed if the assertion fires. Use block scope for related variables and tests. PR-URL: #19054 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 28, 2018

wuweiweiwu force-pushed the test-crypto-binary-default-fixes branch from 32d9450 to 624a417 Compare February 28, 2018 17:40

wuweiweiwu force-pushed the test-crypto-binary-default-fixes branch from 624a417 to 250cb97 Compare February 28, 2018 17:53

jasnell approved these changes Feb 28, 2018

View reviewed changes

wuweiweiwu added 2 commits February 28, 2018 14:07

test: removing assertion message

011629b

The default message will be printed if the assertion fires. PR-URL: nodejs#19054 Reviewed-By: James M Snell <jasnell@gmail.com>

test: scoping tests

b0f8682

Puts related variables and tests in the same scope. PR-URL: nodejs#19054 Reviewed-By: James M Snell <jasnell@gmail.com>

wuweiweiwu force-pushed the test-crypto-binary-default-fixes branch from 250cb97 to b0f8682 Compare February 28, 2018 22:13

watilde approved these changes Mar 1, 2018

View reviewed changes

BridgeAR approved these changes Mar 1, 2018

View reviewed changes

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

apapirovski reviewed Mar 2, 2018

View reviewed changes

test: formatting changes

de967fb

removing extra line break

apapirovski closed this Mar 4, 2018

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

Uh oh!

Conversation

wuweiweiwu commented Feb 28, 2018

Checklist

Affected core subsystem(s)

Uh oh!

watilde commented Feb 28, 2018

Uh oh!

wuweiweiwu commented Feb 28, 2018

Uh oh!

watilde commented Feb 28, 2018

Uh oh!

watilde commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wuweiweiwu commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

watilde commented Mar 1, 2018

Uh oh!

watilde commented Mar 1, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

wuweiweiwu Mar 3, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

Trott commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Mar 2, 2018

Uh oh!

Trott commented Mar 2, 2018

Uh oh!

apapirovski commented Mar 4, 2018

Uh oh!

apapirovski commented Mar 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

watilde commented Feb 28, 2018 •

edited

Loading

wuweiweiwu commented Feb 28, 2018 •

edited

Loading

apapirovski left a comment •

edited

Loading

Trott commented Mar 2, 2018 •

edited

Loading