test: improve zlib tests by jasnell · Pull Request #14455 · nodejs/node

jasnell · 2017-07-24T18:56:45Z

Multiple general improvements to zlib tests

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

Trott · 2017-07-24T20:11:10Z

test/parallel/test-zlib-from-gzip-with-trailing-garbage.js

This loses the check that result is undefined. I'd prefer leaving the old code. I don't think common.expectsError() adds significant value, but the check for result is in fact significant.

Trott

The changes to zlib-random-byte-pipes introduce flakiness under load that is not there currently.

Trott · 2017-07-24T20:32:21Z

With changes in this PR:

$ tools/test.py -j 96 --repeat 96 test/parallel/test-zlib-random-byte-pipes.js
=== release test-zlib-random-byte-pipes ===                                    
Path: parallel/test-zlib-random-byte-pipes
Mismatched noop function calls. Expected exactly 4, actual 5.
    at Object.exports.mustCall (/Users/trott/io.js/test/common/index.js:475:10)
    at Object.<anonymous> (/Users/trott/io.js/test/parallel/test-zlib-random-byte-pipes.js:153:23)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:158:16)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-zlib-random-byte-pipes.js
=== release test-zlib-random-byte-pipes === 
Path: parallel/test-zlib-random-byte-pipes
Mismatched noop function calls. Expected exactly 4, actual 5.
    at Object.exports.mustCall (/Users/trott/io.js/test/common/index.js:475:10)
    at Object.<anonymous> (/Users/trott/io.js/test/parallel/test-zlib-random-byte-pipes.js:153:23)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:158:16)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-zlib-random-byte-pipes.js
[...snip...]

On current master:

$ tools/test.py -j 96 --repeat 96 test/parallel/test-zlib-random-byte-pipes.js 
[00:11|% 100|+  96|-   0]: Done                                
$

Trott · 2017-07-24T20:36:32Z

test/parallel/test-zlib-random-byte-pipes.js

Empirical testing shows that there is no guarantee that this will be called 4 times. Under load at least, it may be called 5 times.

Assuming that discrepancy is valid behavior, seeing that this is all from inp.pipe(gzip).pipe(gunz).pipe(out) and we have a check later for the data event on out, I think this can just be removed. Probably the same for the gunz and gzip checks below.

Was a bit afraid of that. Looking at it, the on('data') events here look entirely unnecessary.

jasnell · 2017-07-24T22:28:20Z

@Trott ... updated

jasnell · 2017-08-02T16:13:40Z

ping @Trott ... I know this needs a rebase, but when you get a moment to take another look I'd appreciate it :-)

comments addressed, dismissing review

Trott · 2017-08-02T16:21:57Z

@nodejs/testing

jasnell · 2017-08-03T18:56:21Z

Rebased!

addaleax

LGTM, two small comments but nothing I’d consider blocking

addaleax · 2017-08-03T19:01:28Z

test/parallel/test-zlib-random-byte-pipes.js

+  write(c) {
+    // Simulate the way that an fs.ReadStream returns false
+    // on *every* write like a jerk, only to resume a
+    // moment later.


Unrelated, but we shouldn’t keep this in the source code.

heh... indeed. completely missed that!

addaleax · 2017-08-03T19:03:03Z

test/parallel/test-zlib-random-byte-pipes.js

+    // on *every* write like a jerk, only to resume a
+    // moment later.
+    this._hasher.update(c);
+    process.nextTick(this.resume.bind(this));


Feel free to replace these with arrow functions if you like :)

jasnell · 2017-08-03T22:35:08Z

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

oops... wrong CI for this PR ... https://ci.nodejs.org/job/node-test-pull-request/9476/

jasnell · 2017-08-04T00:03:16Z

Only failure is unrelated.

PR-URL: #14455 Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2017-08-04T00:11:04Z

Landed in 4e8bc71

PR-URL: #14455 Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 24, 2017

jasnell force-pushed the improve-zlib-tests branch from 2b1fb45 to 6b6140e Compare July 24, 2017 18:58

mscdex added the zlib Issues and PRs related to the zlib subsystem. label Jul 24, 2017

Trott reviewed Jul 24, 2017

View reviewed changes

Trott previously requested changes Jul 24, 2017

View reviewed changes

Trott reviewed Jul 24, 2017

View reviewed changes

jasnell added 9 commits August 3, 2017 11:55

test: improve test-zlib

14dcd38

test: improve test-zlib-write-after-flush

8a8f329

test: improve test-zlib-sync-no-event

5683446

test: improve test-zlib-random-byte-pipes

509498c

test: improve test-zlib-invalid-input

c46527a

test: improve test-zlib-from-string

13e3aa2

test: improve test-zlib-from-gzip

5830895

test: improve test-zlib-from-gzip-with-trailing-garbage

df0be87

[Squash] Fix nits

a905f3b

jasnell force-pushed the improve-zlib-tests branch from daff20c to a905f3b Compare August 3, 2017 18:56

jasnell requested a review from addaleax August 3, 2017 18:56

addaleax approved these changes Aug 3, 2017

View reviewed changes

[Squash] Fix nits

a03fa00

jasnell added a commit that referenced this pull request Aug 4, 2017

test: improve multiple zlib tests

4e8bc71

PR-URL: #14455 Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell closed this Aug 4, 2017

MylesBorins pushed a commit that referenced this pull request Sep 9, 2017

test: improve multiple zlib tests

c76ec71

PR-URL: #14455 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the land-on-v6.x label Sep 20, 2017

MylesBorins pushed a commit that referenced this pull request Sep 20, 2017

test: improve multiple zlib tests

68cf7f0

PR-URL: #14455 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

Uh oh!

Conversation

jasnell commented Jul 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

jasnell commented Aug 2, 2017

Uh oh!

Trott commented Aug 2, 2017

Uh oh!

jasnell commented Aug 3, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 4, 2017

Uh oh!

jasnell commented Aug 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Trott commented Jul 24, 2017 •

edited

Loading

Trott Jul 24, 2017 •

edited

Loading

jasnell commented Aug 3, 2017 •

edited

Loading