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 RegExp second argument to assert.throws #12139

Closed
wants to merge 1 commit into from

Conversation

dave-k
Copy link
Contributor

@dave-k dave-k commented Mar 30, 2017

Add a regular expression argument to assert.throws(Function, RegExp)
in test/addons/make-callback-recurse/test.js.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 30, 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 if CI is green

@Trott
Copy link
Member

Trott commented Mar 30, 2017

@Trott
Copy link
Member

Trott commented Mar 30, 2017

@dave-k You checked the box indicating that your commit message follows the commit guidelines, but it does not. (Looks like you edited the PR message to follow the guidelines.)

If you can fix it and force-push to your branch, then awesome. (If not, that's OK too because whoever lands this can do it at that time.)

@mscdex mscdex added the addons Issues and PRs related to native addons. label Mar 30, 2017
@dave-k
Copy link
Contributor Author

dave-k commented Mar 30, 2017 via email

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Mar 30, 2017

@dave-k According to commit guidelines, you could:

  • Begin the commit title (the first message line) with subsystem prefix (test: ) like in the PR title.
  • Use lower case imperative verb after the subsystem prefix (test: add...) like in the PR title.
  • Insert an empty line after commit title.

So the whole commit message could be:

test: add a second argument to assert.throws()

- a regular expression that matches the entire error message.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

@dave-k ... can you please rebase the commits to get rid of the merge commit?

@dave-k
Copy link
Contributor Author

dave-k commented Apr 4, 2017

@jasnell

do you mean to run?
$ git rebase origin/master

@aqrln
Copy link
Contributor

aqrln commented Apr 4, 2017

@dave-k I believe you'll need to rebase onto upstream/master since your origin/master already contains the junk merge commit. Moreover, I see that you have two identical commits with different commit messages in your master branch (looks like you have done git pull && git push instead of git push -f).

I'd recommend you to do the following:

$ git remote add upstream https://github.com/nodejs/node.git

if you haven't done that yet, and then

$ git fetch upstream
$ git rebase -i upstream/master

Your default text editor will appear (most probably that'll be nano or vi/vim; feel free to ping me if that's the latter and you need any help with using it or with changing the default editor). Change pick to drop for your old commit, and change pick to reword for your new commit since its message has a typo in it (a missing space between two words), save and exit. Then the editor will open once again with the commit message of your new commit. Fix the typo, save and exit. And the last step:

$ git push -f

This way you'll be fine with this PR, but be sure to create new branches for all of your new patches, never open PRs from master. You'll probably need to do

$ git fetch upstream
$ git reset --hard upstream/master

when this PR lands.

- a regular expression that matches the entire error message.
@aqrln
Copy link
Contributor

aqrln commented Apr 4, 2017

@jasnell just notifying you that @dave-k has successfully rebased their commit in case you're waiting for it to land the PR.

@dave-k for your future contributions, take a look at https://github.com/nodejs/node/blob/master/CONTRIBUTING.md for the commit guidelines and Git recipes related to a PR's lifecycle, and happy hacking ;)

@jasnell
Copy link
Member

jasnell commented Apr 4, 2017

vsemozhetbyt pushed a commit that referenced this pull request Apr 7, 2017
- a regular expression that matches the entire error message.

PR-URL: #12139
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
@vsemozhetbyt
Copy link
Contributor

Landed in 943d085
Thank you, @dave-k!

italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
- a regular expression that matches the entire error message.

PR-URL: nodejs#12139
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
- a regular expression that matches the entire error message.

PR-URL: #12139
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
- a regular expression that matches the entire error message.

PR-URL: #12139
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
- a regular expression that matches the entire error message.

PR-URL: nodejs/node#12139
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.