GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — revert: benchmark: test refactoring by addaleax · Pull Request #31722 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Feb 10, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This fixes running benchmark tests. Please 👍 this comment to approve fast-tracking.

Example failure:

=== release test-benchmark-async-hooks ===                   
Path: benchmark/test-benchmark-async-hooks
Error: wrk failed with 1. Output: Usage: wrk <options> <url>                            
  Options:                                            
    -c, --connections <N>  Connections to keep open   
    -d, --duration    <T>  Duration of test           
    -t, --threads     <N>  Number of threads to use   
                                                      
    -s, --script      <S>  Load Lua script file       
    -H, --header      <H>  Add header to request      
        --latency          Print latency statistics   
        --timeout     <T>  Socket/request timeout     
    -v, --version          Print version details      
                                                      
  Numeric arguments may include a SI unit (1k, 1M, 1G)
  Time arguments may include a time unit (2s, 2m, 2h)

    at ChildProcess.<anonymous> (/home/sqrt/src/node/master/benchmark/_http-benchmarkers.js:235:16)
    at Object.onceWrapper (events.js:428:26)
    at ChildProcess.emit (events.js:321:20)
    at maybeClose (internal/child_process.js:1051:16)
    at Socket.<anonymous> (internal/child_process.js:442:11)
    at Socket.emit (events.js:321:20)
    at Pipe.<anonymous> (net.js:673:12)
assert.js:102
  throw new AssertionError(obj);
  ^

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

1 !== 0

    at ChildProcess.<anonymous> (/home/sqrt/src/node/master/test/common/benchmark.js:30: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'
}
Command: out/Release/node /home/sqrt/src/node/master/test/benchmark/test-benchmark-async-hooks.js
[00:00|% 100|+   0|-   1]: Done

(This is back to working after the revert.)

Refs: #31396

Fyi @BridgeAR

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell, mmarchini, and vdeturckheim reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
addaleax added 4 commits February 10, 2020 20:03
@addaleax
Revert "benchmark: remove special test entries"
c11d3d3 
This reverts commit 357230f.
@addaleax
Revert "test: refactor all benchmark tests to use the new test option"
bf5b800 
This reverts commit 78aa348.
@addaleax
Revert "benchmark: add test and all options and improve errors"
9982d41 
This reverts commit dac5795.
@addaleax
Revert "benchmark: refactor helper into a class"
9f36209 
This reverts commit b70741e.
@addaleax addaleax added benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. fast-track PRs proposed for a shorter-than-standard waiting period before landing. labels Feb 10, 2020
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. async_hooks Issues and PRs related to the async hooks subsystem. buffer Issues and PRs related to the buffer subsystem. crypto Issues and PRs related to the crypto subsystem. http Issues and PRs related to the http subsystem. v8 engine Issues and PRs related to the V8 dependency. labels Feb 10, 2020
This was referenced Feb 10, 2020
benchmark: test refactoring #31396
Closed
async_hooks: add executionAsyncResource #30959
Closed
@addaleax addaleax removed v8 engine Issues and PRs related to the V8 dependency. assert Issues and PRs related to the assert subsystem. async_hooks Issues and PRs related to the async hooks subsystem. buffer Issues and PRs related to the buffer subsystem. crypto Issues and PRs related to the crypto subsystem. http Issues and PRs related to the http subsystem. labels Feb 10, 2020
@nodejs-github-bot

nodejs-github-bot commented Feb 10, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29063/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Feb 10, 2020
View reviewed changes
devsnek
devsnek approved these changes Feb 10, 2020
View reviewed changes
vdeturckheim
vdeturckheim approved these changes Feb 10, 2020
View reviewed changes
cjihrig
cjihrig approved these changes Feb 10, 2020
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

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. Out of curiosity, are there any small fixes that could done in place of a full revert?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax

addaleax commented Feb 10, 2020

Copy link
Copy Markdown
Member Author

@cjihrig I tried to debug it, but gave up after half an hour. Somewhere, the duration parameter gets set to a non-integer value, and wrk doesn’t accept that, but that’s all that I could find out.
cjihrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax added a commit that referenced this pull request Feb 10, 2020
@addaleax
Revert "benchmark: remove special test entries"
7cfbc9f 
This reverts commit 357230f.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit that referenced this pull request Feb 10, 2020
@addaleax
Revert "test: refactor all benchmark tests to use the new test option"
aa0a01b 
This reverts commit 78aa348.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit that referenced this pull request Feb 10, 2020
@addaleax
Revert "benchmark: add test and all options and improve errors"
4671d55 
This reverts commit dac5795.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit that referenced this pull request Feb 10, 2020
@addaleax
Revert "benchmark: refactor helper into a class"
5b0308c 
This reverts commit b70741e.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax

addaleax commented Feb 10, 2020

Copy link
Copy Markdown
Member Author

Landed in 2abf0af...5b0308c, thanks for the fast reviews!
vdeturckheim reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Feb 10, 2020
@addaleax
addaleax deleted the revert-31396 branch February 10, 2020 21:36
@BridgeAR

BridgeAR commented Feb 11, 2020

Copy link
Copy Markdown
Member

I just had a look at this. It's due to setting the duration environment variable which is used in TestDoubleBenchmarker. The env was spread into the envs together with duration set when using wrk. That way the string value from the env replaced the original duration set by the options and the benchmark failed.

A fix for this is pretty straight forward by only setting the env value in TestDoubleBenchmarker.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Feb 11, 2020

Copy link
Copy Markdown
Member

There might be more ongoing though. I'll have a look at it later on again.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR mentioned this pull request Feb 12, 2020
benchmark: reland test refactoring #31755
Closed
4 tasks done
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax @codebytere
Revert "benchmark: remove special test entries"
30f55ce 
This reverts commit 357230f.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax @codebytere
Revert "test: refactor all benchmark tests to use the new test option"
5b3c4b3 
This reverts commit 78aa348.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax @codebytere
Revert "benchmark: add test and all options and improve errors"
8974fa7 
This reverts commit dac5795.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@addaleax @codebytere
Revert "benchmark: refactor helper into a class"
819fb76 
This reverts commit b70741e.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
v13.9.0 proposal #31837
Merged
@codebytere codebytere added the dont-land-on-v12.x label Mar 14, 2020
@targos targos added backport-blocked-v12.x and removed dont-land-on-v12.x fast-track PRs proposed for a shorter-than-standard waiting period before landing. labels Apr 20, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax @targos
Revert "benchmark: remove special test entries"
66c5f50 
This reverts commit 357230f.

Refs: nodejs#31396
PR-URL: nodejs#31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax @targos
Revert "benchmark: refactor helper into a class"
4b07730 
This reverts commit b70741e.

Refs: nodejs#31396
PR-URL: nodejs#31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @targos
Revert "benchmark: remove special test entries"
ef80c02 
This reverts commit 357230f.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @targos
Revert "benchmark: refactor helper into a class"
cf2ca11 
This reverts commit b70741e.

Refs: #31396
PR-URL: #31722
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
+2 more reviewers
@devsnek devsnek devsnek approved these changes
@vdeturckheim vdeturckheim vdeturckheim approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@addaleax @nodejs-github-bot @BridgeAR @jasnell @cjihrig @devsnek @vdeturckheim @codebytere @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.