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

Investigate flaky test-benchmark-misc #31372

Closed
AshCripps opened this issue Jan 15, 2020 · 10 comments · Fixed by #31377
Closed

Investigate flaky test-benchmark-misc #31372

AshCripps opened this issue Jan 15, 2020 · 10 comments · Fixed by #31377
Labels
benchmark Issues and PRs related to the benchmark subsystem. confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests.

Comments

@AshCripps
Copy link
Member

  • Version: Master
  • Platform: Custom Suites Freestyle
  • Subsystem: benchmark
internal/readline/utils.js:163
  return str.replace(ansi, '');
             ^

TypeError: Cannot read property 'replace' of undefined
    at stripVTControlCharacters (internal/readline/utils.js:163:14)
    at getStringWidth (internal/readline/utils.js:71:11)
    at main (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/benchmark/misc/getstringwidth.js:24:5)
    at /home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/benchmark/common.js:40:28
    at processTicksAndRejections (internal/process/task_queues.js:79:11)
assert.js:102
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

This has been failing node-daily-master since 10-01-2020.
https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1802/
https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1803/
https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1804/
https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1805/
https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1806/

@sam-github
Copy link
Contributor

Not flaky, flat-out broken, fails on master, in ubuntu:

% ./out/Release/node test/benchmark/test-benchmark-misc.js 
internal/readline/utils.js:162
  return str.replace(ansi, '');
             ^

TypeError: Cannot read property 'replace' of undefined
    at stripVTControlCharacters (internal/readline/utils.js:162:14)
    at getStringWidth (internal/readline/utils.js:71:11)
    at main (/home/sam/w/core/node/benchmark/misc/getstringwidth.js:24:5)
    at /home/sam/w/core/node/benchmark/common.js:40:28
    at processTicksAndRejections (internal/process/task_queues.js:79:11)
assert.js:102
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

    at ChildProcess.<anonymous> (/home/sam/w/core/node/test/common/benchmark.js:35:12)
    at ChildProcess.emit (events.js:321:20)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}

@sam-github sam-github added benchmark Issues and PRs related to the benchmark subsystem. confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests. labels Jan 16, 2020
@Trott
Copy link
Member

Trott commented Jan 16, 2020

Didn't look too closely, but on the surface, appears to be a victim of recent readline changes. Perhaps fe05818 and/or 5baae14? /ping @BridgeAR

@Trott
Copy link
Member

Trott commented Jan 16, 2020

(By the way, if this turns out to be a genuine bug in Node.js core, I never want to hear anyone ever say again "Why do we have tests for the benchmarks?")

@sam-github
Copy link
Contributor

@Trott thanks

I'm doing to do a manual bisect, but my underpowered machine is making slow progress.

@Trott
Copy link
Member

Trott commented Jan 16, 2020

I'm doing to do a manual bisect, but my underpowered machine is making slow progress.

You're almost certainly ahead of me. I'm doing work while my machine compiles the master branch....

@BridgeAR
Copy link
Member

BridgeAR commented Jan 16, 2020

It's 539df73. The issue is the benchmark test. It has the type set to an empty string and there is no default value in the corresponding benchmark file. It passes undefined to the function. That was coerced to a string originally but no internal implementation passes anything through besides a string.

The solution is to use a default value in the benchmark or to pass through a legit value.

@BridgeAR
Copy link
Member

The way our benchmark tests work is less than ideal. I won't find time for it right away but I'll look into rewriting the implementation so that we do not have to maintain our tests anymore.

@Trott
Copy link
Member

Trott commented Jan 16, 2020

The way our benchmark tests work is less than ideal. I won't find time for it right away but I'll look into rewriting the implementation so that we do not have to maintain our tests anymore.

Yeah, instead of having default values, it may be best if the test is smart enough to determine the possible options and pop the first value off the default list, or something like that.

@Trott
Copy link
Member

Trott commented Jan 16, 2020

Quick fix: #31377

@BridgeAR
Copy link
Member

Yeah, instead of having default values, it may be best if the test is smart enough to determine the possible options and pop the first value off the default list, or something like that.

Exactly that 👍

@Trott Trott closed this as completed in d2683ed Jan 16, 2020
MylesBorins pushed a commit that referenced this issue Jan 16, 2020
This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

PR-URL: #31377
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
codebytere pushed a commit that referenced this issue Mar 14, 2020
This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

PR-URL: #31377
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

PR-URL: #31377
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
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. confirmed-bug Issues with confirmed bugs. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants