-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: refactor of zlib-from-gzip-with-trailing-garbage #10674
Conversation
|
||
zlib.gunzip(data, common.mustCall((err, result) => { | ||
zlib.gunzip(data, common.mustCall((err) => { | ||
assert(err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would likely be worthwhile validating the details of the error reported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also recommend restoring result
as an argument and adding an assertion for it. So all together, maybe something like this?:
assert(err instanceof Error);
assert.strictEqual(err.code, 'Z_DATA_ERROR');
assert.strictEqual(err.message, 'unknown compression method');
assert.strictEqual(result, undefined);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New commit pushed up with these suggestions included. Thanks to the both of you for the help. Long time listener, first time committer. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one suggestion
|
||
zlib.gunzip(data, common.mustCall((err, result) => { | ||
zlib.gunzip(data, common.mustCall((err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestions here as above. Restore result
, add an assertion to confirm it is undefined
, and do some more rigorous checking on err
.
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * removed unused arguments
* restore result args and validate response * validate error message and code
@jasnell I see one test failed... any suggestion on how to fix this? |
@leftynaut |
Bug here: nodejs/github-bot#108 |
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: nodejs#10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Landed in 39c4af5. |
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: nodejs#10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: nodejs#10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: nodejs#10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: nodejs#10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: #10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* use assert.strictEqual instead of assert.equal * add RegExp in second argument of assert.throws * validate error message and code PR-URL: #10674 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Checklist