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 to assert in zlib-write-after-close #11482

Closed
wants to merge 3 commits into from

Conversation

SgiobairOg
Copy link
Contributor

@SgiobairOg SgiobairOg commented Feb 21, 2017

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 21, 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 Feb 21, 2017

@Trott
Copy link
Member

Trott commented Feb 21, 2017

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 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 mscdex added the zlib Issues and PRs related to the zlib subsystem. label Feb 21, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor 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?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

function() {
unzip.write(out);
},
/^Error: zlib binding closed$/
Copy link
Member

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Feb 22, 2017

@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
Copy link
Member

hiroppy commented Feb 23, 2017

@SgiobairOg
Copy link
Contributor 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
Copy link
Member

lpinca commented Feb 23, 2017

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

removed excess spaces from line 9
@lpinca
Copy link
Member

lpinca commented Feb 23, 2017

@SgiobairOg
Copy link
Contributor 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
Copy link
Member

lpinca commented Feb 23, 2017

@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
PR-URL: #11482
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 24, 2017

Landed in 817b28b

@jasnell jasnell closed this Feb 24, 2017
addaleax pushed a commit that referenced this pull request Feb 24, 2017
PR-URL: #11482
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11482
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11482
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11482
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11482
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants