Skip to content

test: refactor test-crypto test files - #8597

Closed
griffithtp wants to merge 1 commit into
nodejs:masterfrom
griffithtp:code-and-learn-amsterdam-2016
Closed

test: refactor test-crypto test files#8597
griffithtp wants to merge 1 commit into
nodejs:masterfrom
griffithtp:code-and-learn-amsterdam-2016

Conversation

@griffithtp

@griffithtpgriffithtp commented Sep 17, 2016

Copy link
Copy Markdown
Checklist
  • make lint
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Description of change

Updated ./test/parallel/test-crypto*

  • var to const
  • assert.equal to assert.StrictEqual, same with notEqual
  • wrap callback with common.mustCall()

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM pending CI

@mscdexmscdex added the crypto Issues and PRs related to the crypto subsystem. label Sep 17, 2016

@jasnelljasnell left a comment

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.

LGTM. when landing the commits will need to be squashed and the commit messages should be fixed up.

@jasnell

Copy link
Copy Markdown
Member

@griffithtp ... thank you for this. Just to point out, the contributing.md file in the node.js project root directly includes the guidelines for commit message formatting. The person who lands this can clean these commits up but if you'd like to give it a go yourself that would be great!

@griffithtp

Copy link
Copy Markdown
Author

@jasnell apologies for this. Rookie mistake.
If I run git rebase -i HEAD~3 now and push again would this fix it and squash?

@jasnell

Copy link
Copy Markdown
Member

Yep. Specifically, the steps are git rebase -i HEAD~3, squash, redo the commit message, then force push to your branch. This PR will automatically pick up the changes.

@griffithtp
griffithtpforce-pushed the code-and-learn-amsterdam-2016 branch 2 times, most recently from 57d10f6 to 435437fCompareSeptember 21, 2016 10:11
@griffithtp

Copy link
Copy Markdown
Author

Cool, thanks James @jasnell !
Is there a list of TODOs for newbie contributors we can follow to just crunch the tasks to do.
Otherwise I can carry on with the same principles from the conference last week?

@addaleax

Copy link
Copy Markdown
Member

Is there a list of TODOs for newbie contributors we can follow to just crunch the tasks to do.

Well, there’s https://github.com/nodejs/node/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+contribution%22 if you’re feeling up for one of those?

Otherwise I can carry on with the same principles from the conference last week?

I don’t want to steal your thunder or anything, but… keep in mind that the kind of tasks we were doing was chosen specifically to have some good contributions to get used to the process. You can definitely look through the test files and watch for anything that you would consider a significant improvement!

Also, things that we just always need are more benchmarks + more tests (code coverage on Linux).

@jasnell

Copy link
Copy Markdown
Member

... and doc fixes! we always need doc fixes!

@jasnell

Copy link
Copy Markdown
Member

@addaleax

Copy link
Copy Markdown
Member

make lint output:

/home/sqrt/src/node/test/parallel/test-crypto-hash.js
50:14 error Function argument in column 14, expected in 20 align-function-arguments
87:16 error Function argument in column 16, expected in 22 align-function-arguments
/home/sqrt/src/node/test/parallel/test-crypto-hmac.js
71:18 error Function argument in column 18, expected in 24 align-function-arguments
237:18 error Function argument in column 18, expected in 24 align-function-arguments
✖ 4 problems (4 errors, 0 warnings)

@lpinca

Copy link
Copy Markdown
Member

@griffithtp can you take a look at the lint issues?

@rvagg
rvaggforce-pushed the master branch 2 times, most recently from c133999 to 83c7a88CompareOctober 18, 2016 17:02
@griffithtp
griffithtpforce-pushed the code-and-learn-amsterdam-2016 branch from 435437f to 92c7917CompareOctober 28, 2016 23:50
@griffithtp

Copy link
Copy Markdown
Author

Apologies for late reply.
Have updated indent and ran make lint as suggested by @addaleax
then make -j2 test after rebase from latest upstream master

@jasnelljasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@fhinkel

Copy link
Copy Markdown
Member

ping @griffithtp Sorry that this PR didn't get more attention. Do you want to rebase (again, sorry!) and ping me? Then I'll start the CI and we can land this. Thanks, sorry for the extra work.

@TrottTrott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Aug 13, 2017
* invoke callback with `common.mustCall()` in test-crypto-hash
* order module declarations aphabetically per test-writing-guide
@Trott
Trottforce-pushed the code-and-learn-amsterdam-2016 branch from 92c7917 to e650c6fCompareAugust 13, 2017 18:36
@Trott

Copy link
Copy Markdown
Member

I took the liberty of rebasing, resolving conflicts, and pushing to your branch. Hope that's OK! Given the staleness of the PR, I figured it probably wasn't a problem. I also edited the commit message a bit to reflect commit message guidelines and the narrower scope of the PR now that it's been rebased (because some of the changes in this PR were already done elsewhere).

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

@Trott

Copy link
Copy Markdown
Member

Landed in 1df9340.

Thanks for the contribution! 🎉

@TrottTrott closed this Aug 13, 2017
Trott pushed a commit that referenced this pull request Aug 13, 2017
* invoke callback with `common.mustCall()` in test-crypto-hash
* order module declarations aphabetically per test-writing-guide
PR-URL: #8597
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 9, 2017
* invoke callback with `common.mustCall()` in test-crypto-hash
* order module declarations aphabetically per test-writing-guide
PR-URL: #8597
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
* invoke callback with `common.mustCall()` in test-crypto-hash
* order module declarations aphabetically per test-writing-guide
PR-URL: #8597
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Sep 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.cryptoIssues and PRs related to the crypto subsystem.stalledIssues and PRs that are stalled.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@griffithtp@jasnell@addaleax@lpinca@fhinkel@Trott@cjihrig@mscdex@MylesBorins@nodejs-github-bot