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: Add regex check to error expectation #10038

Closed
wants to merge 2 commits into from
Closed

test: Add regex check to error expectation #10038

wants to merge 2 commits into from

Conversation

samshull
Copy link

@samshull samshull 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

Description of change

Creating a buffer from a number should throw an error with a message
that describes the issue.

Creating a buffer from a number should throw an error with a message
that describes the issue.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. buffer Issues and PRs related to the buffer subsystem. labels Dec 1, 2016
@MylesBorins
Copy link
Contributor

@samshull just so you know you can force push over branches on github to update a PR. This will allow you to make changes to a commit without needing to open a new PR

@samshull
Copy link
Author

samshull commented Dec 2, 2016

@thealphanerd, yes, I understand force push, I created a second branch because I had multiple PRs that could not be submitted from the same branch and could not create a branch off a branch because of the fork requirements of github.

Edit: I had two PRs, but different code in each.

@@ -8,7 +8,7 @@ assert.doesNotThrow(function() {

assert.throws(function() {
Buffer.from(10, 'hex');
});
}, /argument must not be a number/);
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to test the whole error message:

/^TypeError: "value" argument must not be a number$/

Copy link
Author

Choose a reason for hiding this comment

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

What is the benefit of full text message checking?

Copy link
Member

Choose a reason for hiding this comment

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

@samshull it helps tracking changes in the error message/type as those changes are breaking changes (semver-major).

@samshull
Copy link
Author

@jasnell change submitted

@italoacasas
Copy link
Contributor

italoacasas commented Dec 18, 2016

@Trott
Copy link
Member

Trott commented Dec 22, 2016

@jasnell It appears that your comment has been addressed. Can you take a look and, if so, please update your review so we can land this? Thanks!

@italoacasas
Copy link
Contributor

Landed bae695f

italoacasas pushed a commit that referenced this pull request Dec 22, 2016
Creating a buffer from a number should throw an error with a message
that describes the issue.

PR-URL: #10038
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Dec 26, 2016
Creating a buffer from a number should throw an error with a message
that describes the issue.

PR-URL: #10038
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 27, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
Creating a buffer from a number should throw an error with a message
that describes the issue.

PR-URL: #10038
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
Creating a buffer from a number should throw an error with a message
that describes the issue.

PR-URL: #10038
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants