Skip to content

test: add v8, vm, streams test benchmarks#22335

Closed
lundibundi wants to merge 3 commits intonodejs:masterfrom
lundibundi:add-more-test-benchmarks
Closed

test: add v8, vm, streams test benchmarks#22335
lundibundi wants to merge 3 commits intonodejs:masterfrom
lundibundi:add-more-test-benchmarks

Conversation

@lundibundi
Copy link
Member

@lundibundi lundibundi commented Aug 15, 2018 •
edited
Loading

Checklist

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 15, 2018
@Trott Trott dismissed a stale review August 15, 2018 18:56

might be ok, need to look more closely

@Trott
Copy link
Member

Trott commented Aug 15, 2018

I'm concerned that allowMultiple will be used to bypass the configuration requirements rather than enforce them.

Trott
Trott requested changes Aug 15, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

OK, looking more closely: I understand the reasoning for wanting to add allowMultiple but I think the downside is much greater than the upside. The benchmark tests are not exhaustive. They are minimal and that is definitely on purpose. (Otherwise, they would take forever to run.) I think it would be best to make these conform to the existing tests and not add allowMultiple which will probably end up being abused at a later date.

@lundibundi
Copy link
Member Author

lundibundi commented Aug 15, 2018

Hmm, I thought about it being abused but assumed that such tests don't get changed often, therefore, they can be validated more thoroughly to avoid such abuse.
Though I understand your reasoning (I specifically had it as a separate commit to make sure we go through it). Also, we may implement it as a number of runs to avoid disabling the check but just extend it for some tests.
For now, though, I'll then change it to just be a minimal implementation.

@lundibundi lundibundi force-pushed the add-more-test-benchmarks branch from e7f5897 to 03635c2 Compare August 15, 2018 19:56
@lundibundi lundibundi changed the title Add v8,vm test benchmarks Add v8, vm, streams test benchmarks Aug 15, 2018
Trott
Trott approved these changes Aug 15, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

BridgeAR
BridgeAR reviewed Aug 17, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Code LGTM but I am not a fan of adding these tests in general. But I am not going to block this either.

@lundibundi
Copy link
Member Author

lundibundi commented Aug 17, 2018

@BridgeAR is there a special place for these? I just put them where other such tests were.

@lundibundi lundibundi changed the title Add v8, vm, streams test benchmarks test: add v8, vm, streams test benchmarks Aug 18, 2018
@Trott
Copy link
Member

Trott commented Aug 18, 2018

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

@Trott Trott force-pushed the add-more-test-benchmarks branch from 03635c2 to f56e2d6 Compare August 18, 2018 19:58
@Trott
Copy link
Member

Trott commented Aug 18, 2018

Rebased so fix jinja LICENSE thing in CI. CI: https://ci.nodejs.org/job/node-test-pull-request/16554/

@Trott
Copy link
Member

Trott commented Aug 19, 2018

Resume build: https://ci.nodejs.org/job/node-test-pull-request/16574/

This was referenced Aug 20, 2018
Closed
Closed
@addaleax
Copy link
Member

addaleax commented Aug 24, 2018

Landed in d495e40...902fd40

@addaleax addaleax closed this Aug 24, 2018
addaleax pushed a commit that referenced this pull request Aug 24, 2018
@lundibundi @addaleax
test: add v8 benchmark test
cac4e6d 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Aug 24, 2018
@lundibundi @addaleax
test: add vm benchmark test
5d703ec 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Aug 24, 2018
@lundibundi @addaleax
test: add streams benchmark test
902fd40 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@lundibundi lundibundi deleted the add-more-test-benchmarks branch August 25, 2018 19:14
addaleax pushed a commit that referenced this pull request Aug 27, 2018
@lundibundi @addaleax
test: add v8 benchmark test
a196107 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
@lundibundi @addaleax
test: add vm benchmark test
8e91dbd 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
@lundibundi @addaleax
test: add streams benchmark test
865e43e 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 3, 2018
@lundibundi @targos
test: add v8 benchmark test
400aac8 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 3, 2018
@lundibundi @targos
test: add vm benchmark test
ef60a8d 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 3, 2018
@lundibundi @targos
test: add streams benchmark test
1e7deb7 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@lundibundi @targos
test: add v8 benchmark test
594b237 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 6, 2018
@lundibundi @targos
test: add vm benchmark test
caaac3f 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 6, 2018
@lundibundi @targos
test: add streams benchmark test
c91472e 
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lundibundi @Trott @addaleax @jasnell @BridgeAR @nodejs-github-bot