FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add a RegExp by zjyw · Pull Request #10058 · nodejs/node · GitHub

/ node Public

test: add a RegExp - #10058

Closed
zjyw wants to merge 1 commit into
nodejs:masterfrom
zjyw:master
ConversationCommits (1)ChecksFiles changed
Closed

test: add a RegExp#10058
zjyw wants to merge 1 commit into
nodejs:masterfrom
zjyw:master

Conversation

zjyw commented Dec 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Add a RegExp as a second argument to assert.throws().

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
drifkin reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-dns-regress-7070.js Outdated

drifkin Dec 1, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this line was committed by accident?

zjyw Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes. Thanks for catching it. Now it is fixed.

addaleax requested changes Dec 2, 2016
View reviewed changes
Comment thread test/parallel/test-dns-regress-7070.js Outdated

addaleax Dec 2, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These lines are longer than 80 characters, which is something we want to avoid – could you move the second argument to a new line (and align that below the first argument)?

cjihrig Dec 3, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you're checking the entire error message, you can add ^ and $ to the regular expression as well.

zjyw commented Dec 6, 2016

Copy link
Copy Markdown
Contributor Author

thanks for the suggestions. made the changes.

jasnell approved these changes Dec 6, 2016
View reviewed changes

jasnell 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The changes LGTM but the commits should be squashed and a bit more descriptive.. for instance:

test: test error messages in test-dns-regress-7070.js

cjihrig reviewed Dec 6, 2016
View reviewed changes
Comment thread test/parallel/test-dns-regress-7070.js Outdated

cjihrig Dec 6, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks like some merge conflicts got through.

zjyw commented Dec 6, 2016

Copy link
Copy Markdown
Contributor Author

It took me several commits to get it right. I think it should be good now. Let me know if there is anything else I need to change.

lpinca reviewed Dec 11, 2016
View reviewed changes
Comment thread test/parallel/test-dns-regress-7070.js Outdated

lpinca Dec 11, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@zjyw can you please add a final new line? The linter will fail without it.

zjyw Dec 11, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yeah. just added it. thanks for pointing it out. i was wondering about it after my last commit.

Fishrock123 commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

dont-land-on-v7.x seems correct here. - nodejs/github-bot#100

lpinca commented Dec 16, 2016

Copy link
Copy Markdown
Member

@Fishrock123 why this shouldn't land on v7.x ?

lpinca commented Dec 16, 2016

Copy link
Copy Markdown
Member

@addaleax LGTY?

targos commented Dec 16, 2016

Copy link
Copy Markdown
Member

@lpinca The changes don't apply cleanly (even on master)

lpinca commented Dec 16, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@targos I see, the green merge button tricked me, thanks.

zjyw commented Dec 16, 2016

Copy link
Copy Markdown
Contributor Author

@targos I am new to the node.js change system. When you say it won't apply cleanly, do you mean the unit test will fail or something else?

gibfahn commented Dec 16, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@zjyw He means that there are merge conflicts when he tried to apply this PR as a series of patches (as described here). If you rebase against master it should fix the issues.

addaleax approved these changes Dec 16, 2016
View reviewed changes

addaleax 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, lgtm!

lpinca commented Dec 16, 2016

Copy link
Copy Markdown
Member

@zjyw if you could rebase this PR against master and optionally squash your commits that would be awesome.

Fishrock123 commented Dec 16, 2016

Copy link
Copy Markdown
Contributor

Next time please try to do a backport before removing the label, thanks.

This does not apply cleanly on v7.x

targos commented Dec 16, 2016

Copy link
Copy Markdown
Member

@Fishrock123 I tried and it does apply cleanly if the commits are squashed.
Maybe I don't understand correctly the purpose of the dont-land-on- labels. What I read is "do not land", as an imperative mood. When I put the label on a PR, it is to indicate that the commit should not go into that branch, even if it applies cleanly.

zjyw commented Dec 18, 2016

Copy link
Copy Markdown
Contributor Author

Rebased and squashed using 'git rebase -i HEAD~7'. Let me know if I did it right.

test: test error messages in test-dns-regress-7070
ccf4b3c 
Add a RegExp as a second argument to assert.throws().

targos commented Dec 19, 2016

Copy link
Copy Markdown
Member

@zjyw it seems something went wrong. I went ahead and fixed it. Here is what I did, with the upstream remote pointing to this repo (nodejs/node):

  • git fetch upstream
  • git rebase -i upstream/master
  • Pick the first commit and squash the others:
  • Fix the conflicts (just always kept the HEAD part) until the rebase is done.
  • make test to check everything is fine
  • git push --force-with-lease zjyw HEAD:master

targos commented Dec 19, 2016

Copy link
Copy Markdown
Member

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

gibfahn mentioned this pull request Dec 19, 2016
Closed

zjyw commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

@targos thanks for help!

Trott commented Dec 22, 2016

Copy link
Copy Markdown
Member

CI failure on FreeBSD seems entirely unrelated to this change, but just to be sure...

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

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 24, 2016
test: test error messages in test-dns-regress-7070
be6de1a 
Add a RegExp as a second argument to assert.throws().

PR-URL: nodejs#10058
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Trott commented Dec 24, 2016

Copy link
Copy Markdown
Member

Landed in be6de1a.
Thanks for the contribution! 🎉

Trott closed this Dec 24, 2016
targos pushed a commit that referenced this pull request Dec 26, 2016
test: test error messages in test-dns-regress-7070
25b76a4 
Add a RegExp as a second argument to assert.throws().

PR-URL: #10058
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins mentioned this pull request Dec 27, 2016
Merged
evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
test: test error messages in test-dns-regress-7070
1e6319a 
Add a RegExp as a second argument to assert.throws().

PR-URL: #10058
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
test: test error messages in test-dns-regress-7070
e9b2325 
Add a RegExp as a second argument to assert.throws().

PR-URL: #10058
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
+1 more reviewer
drifkin drifkin left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL