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: fix flaky test-benchmark-fs #17885

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 27, 2017

Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test benchmark fs

Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.
@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Dec 27, 2017
@Trott
Copy link
Member Author

Trott commented Dec 27, 2017

Sample CI failure this will fix: https://ci.nodejs.org/job/node-test-commit-linux/15217/nodes=alpine36-container-x64/consoleText

not ok 155 parallel/test-benchmark-fs
  ---
  duration_ms: 15.866
  severity: fail
  stack: |-
    
    fs/bench-readdir.js
    fs/bench-readdir.js n=1: 58.80744384624206
    
    fs/bench-readdirSync.js
    fs/bench-readdirSync.js n=1: 2,460.6844147631223
    
    fs/bench-realpath.js
    fs/bench-realpath.js pathType="relative" n=1: 23.852785948152057
    
    fs/bench-realpathSync.js
    fs/bench-realpathSync.js pathType="relative" n=1: 2,926.6208357843784
    
    fs/bench-stat.js
    fs/bench-stat.js statType="fstat" n=1: 138.54832939809896
    
    fs/bench-statSync.js
    fs/bench-statSync.js statSyncType="fstatSync" n=1: 2,312.764377877946
    
    fs/read-stream-throughput.js
    fs/read-stream-throughput.js size=1 filesize=1024 encodingType="buf": 0.0003594309105535539
    
    fs/readFileSync.js
    fs/readFileSync.js n=1: 4,224.989120653015
    
    fs/readfile.js
    /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine36-container-x64/benchmark/common.js:198
        throw new Error('called end() with operation count <= 0');
        ^
    
    Error: called end() with operation count <= 0
        at Benchmark.end (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine36-container-x64/benchmark/common.js:198:11)
        at Timeout._onTimeout (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine36-container-x64/benchmark/fs/readfile.js:30:11)
        at ontimeout (timers.js:454:11)
        at tryOnTimeout (timers.js:304:5)
        at Timer.listOnTimeout (timers.js:264:5)
    assert.js:43
      throw new errors.AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: 1 strictEqual 0
        at ChildProcess.child.on (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine36-container-x64/test/common/benchmark.js:25:12)
        at ChildProcess.emit (events.js:131:13)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:209:12)
  ...

@Trott
Copy link
Member Author

Trott commented Dec 27, 2017

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

woo :)

@apapirovski
Copy link
Member

Can we do this for test-benchmark-module too? I saw it fail somewhere recently for the same reason.

@Trott
Copy link
Member Author

Trott commented Dec 27, 2017

Can we do this for test-benchmark-module too? I saw it fail somewhere recently for the same reason.

@apapirovski I'm not seeing how it's possible for test-benchmark-module and I'm unable to make it fail locally. Any chance you're mistaken and it was some other test? Or are you certain is was that test?

@Trott
Copy link
Member Author

Trott commented Dec 28, 2017

By the way, this fixes one issue with the fs benchmark tests, but there remains a second issue: In benchmark/fs/write-stream-throughput.js, it is possible (if dur is very short) that bench.end() gets called before bench.start(), and that will throw an error. Example at https://ci.nodejs.org/job/node-test-commit-aix/11455/nodes=aix61-ppc64/consoleText:

fs/write-stream-throughput.js
    /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/benchmark/common.js:192
        throw new Error('called end without start');
        ^
    
    Error: called end without start
        at Benchmark.end (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/benchmark/common.js:192:11)
        at WriteStream.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/benchmark/fs/write-stream-throughput.js:57:11)
        at WriteStream.emit (events.js:136:15)
        at finishMaybe (_stream_writable.js:633:14)
        at endWritable (_stream_writable.js:641:3)
        at WriteStream.Writable.end (_stream_writable.js:583:5)
        at Timeout._onTimeout (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/benchmark/fs/write-stream-throughput.js:46:7)
        at ontimeout (timers.js:454:11)
        at tryOnTimeout (timers.js:304:5)
        at Timer.listOnTimeout (timers.js:264:5)
    assert.js:43
      throw new errors.AssertionError(obj);
      ^

Trott added a commit to Trott/io.js that referenced this pull request Dec 28, 2017
Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.

PR-URL: nodejs#17885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@Trott
Copy link
Member Author

Trott commented Dec 28, 2017

Landed in 9c00f07

@Trott Trott closed this Dec 28, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.

PR-URL: #17885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.

PR-URL: #17885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.

PR-URL: #17885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
Some benchmarks may return 0 operations with the new very short duration
provided by the test program. Set environment variable to allow that.

PR-URL: #17885
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@Trott Trott deleted the fix-flaky-test-benchmark-fs branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants