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: refactor assert.equal, update assert.throws #9914

Closed
wants to merge 4 commits into from

Conversation

prietoguy
Copy link

@prietoguy prietoguy commented Dec 1, 2016

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

test

Kudos

@Fishrock123
@bengl
@jasnell
@cjihrig

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

The commit message's subsystem prefix should be all lowercase.


assert.throws(function() {
crypto.createDiffieHellman([0x1, 0x2]);
});
}, /First argument should be number, string or Buffer/);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're matching the entire error, you can add ^ and $ to the regular expression.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, will make that change. Thanks!


assert.throws(function() {
crypto.createDiffieHellman([0x1, 0x2]);
});
}, /^First argument should be number, string or Buffer$/);
Copy link
Member

Choose a reason for hiding this comment

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

I think you also need to add the TypeError part or the test will fail:

/^TypeError: First argument should be number, string or Buffer$/

@Trott
Copy link
Member

Trott commented Dec 9, 2016

Thanks for the work on this!

Two of the three files modified here have lint errors and one of those two fails to run successfully because of the issue identified by @lpinca.

The changes in these three files are unrelated to each other for the most part, so I'm going to remove the changes to the files that are failing, run CI, and land the one file with the non-problematic changes (unless someone beats me to it and lands it first).

If you want to still try to get the changes to the other two files in, that's great! Create a new branch and open a separate pull request. Thanks!

@Trott
Copy link
Member

Trott commented Dec 9, 2016

Opened #10190 rather than force-pushing here. Going to close this one. (If #10190 lands, the commit will show you as the author.)

@Trott Trott closed this Dec 9, 2016
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.

7 participants