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

test: add regex to assert in zlib-write-after-close by SgiobairOg · Pull Request #11482 · nodejs/node · GitHub

/ node Public

test: add regex to assert in zlib-write-after-close - #11482

Closed
SgiobairOg wants to merge 3 commits into
nodejs:masterfrom
SgiobairOg:master
ConversationCommits (3)ChecksFiles changed
Closed

test: add regex to assert in zlib-write-after-close#11482
SgiobairOg wants to merge 3 commits into
nodejs:masterfrom
SgiobairOg:master

Conversation

SgiobairOg commented Feb 21, 2017 •
edited
Loading

Copy link
Copy Markdown

docs for assert.throws() allows the addition of a regular expression argument.

test/parallel/test-zlib-write-after-close.js line 9 contains an assert.throws() call which only has a single argument.

Per NodeToDo have added regex argument to the assert.throws() call to validate received error message.

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

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 21, 2017
Trott approved these changes Feb 21, 2017
View reviewed changes

Trott 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

LGTM if CI is green

Trott commented Feb 21, 2017

Copy link
Copy Markdown
Member

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

Trott commented Feb 21, 2017

Copy link
Copy Markdown
Member

Nit: The first line of the commit message should be no more than 50 chars. However, whoever lands this can fix that. Feel free to fix it yourself if you'd like to save them a few keystrokes. Either way.

SgiobairOg changed the title test: add regex argument to assert throw in /test/parallel/test-zlib-write-after-close.js test: add regex to assert in zlib-write-after-close Feb 21, 2017
mscdex added the zlib Issues and PRs related to the zlib subsystem. label Feb 21, 2017
cjihrig approved these changes Feb 21, 2017
View reviewed changes

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

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

LGTM with one nit. This can be a single line change. Can you just add the regular expression without moving around the rest of the code.

SgiobairOg commented Feb 22, 2017

Copy link
Copy Markdown
Author

I can make that change @cjihrig. Would it be worthwhile to change to the fat arrow syntax used in the docs while I'm in there? What is the procedure once the change is made? New pull request?

jasnell approved these changes Feb 22, 2017
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

LGTM with a suggestion. Feel free to ignore, however.

Comment thread test/parallel/test-zlib-write-after-close.js Outdated
function() {
unzip.write(out);
},
/^Error: zlib binding closed$/

jasnell Feb 22, 2017

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

this could be updated to use the new common.expectsError()

Trott commented Feb 22, 2017

Copy link
Copy Markdown
Member

@SgiobairOg If you want to make the change suggested by @cjihrig, make it on this branch and push it there. That will update this PR. No need to open a new PR (and in fact, please don't!).

hiroppy commented Feb 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

SgiobairOg commented Feb 23, 2017

Copy link
Copy Markdown
Author

I'm new to Jenkins. I can't seem to figure out what the actual errors are. How do I find out what I need to correct?

lpinca commented Feb 23, 2017

Copy link
Copy Markdown
Member

@SgiobairOg There seem to be only lint issues. Run make lint to see what's wrong.

test: remove space in paren for lint
49e528d 
removed excess spaces from line 9

lpinca commented Feb 23, 2017

Copy link
Copy Markdown
Member

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

SgiobairOg commented Feb 23, 2017

Copy link
Copy Markdown
Author

Alright, down to one fail. Is test/arm saying that whatever I've done here doesn't work on arm-based chips? Where would I track down details on this one.

lpinca commented Feb 23, 2017

Copy link
Copy Markdown
Member

@SgiobairOg no fails, CI is 100% green 🎉. If you click on details you'll see that tests passed.

jasnell pushed a commit that referenced this pull request Feb 24, 2017
test: throw check in test-zlib-write-after-close
817b28b 
PR-URL: #11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

jasnell commented Feb 24, 2017

Copy link
Copy Markdown
Member

Landed in 817b28b

jasnell closed this Feb 24, 2017
addaleax pushed a commit that referenced this pull request Feb 24, 2017
test: throw check in test-zlib-write-after-close
8fdb6c2 
PR-URL: #11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
italoacasas mentioned this pull request Feb 25, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
test: throw check in test-zlib-write-after-close
ae4515d 
PR-URL: #11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
test: throw check in test-zlib-write-after-close
e2af074 
PR-URL: #11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
test: throw check in test-zlib-write-after-close
6202f14 
PR-URL: #11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
test: throw check in test-zlib-write-after-close
f0eee61 
PR-URL: #11482
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
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

jasnell jasnell approved these changes
Trott Trott approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL