FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add v8, vm, streams test benchmarks by lundibundi · Pull Request #22335 · nodejs/node · GitHub

/ node Public

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

Closed
lundibundi wants to merge 3 commits into
nodejs:masterfrom
lundibundi:add-more-test-benchmarks
ConversationCommits (3)ChecksFiles changed
Closed

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

Conversation

lundibundi commented Aug 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Member
Checklist

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

might be ok, need to look more closely

Trott commented Aug 15, 2018

Copy link
Copy Markdown
Member

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

Trott requested changes Aug 15, 2018
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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 force-pushed the add-more-test-benchmarks branch from e7f5897 to 03635c2 Compare August 15, 2018 19:56
lundibundi changed the title Add v8,vm test benchmarks Add v8, vm, streams test benchmarks Aug 15, 2018
Trott approved these changes Aug 15, 2018
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

BridgeAR reviewed Aug 17, 2018
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lundibundi commented Aug 17, 2018

Copy link
Copy Markdown
Member Author

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

lundibundi changed the title Add v8, vm, streams test benchmarks test: add v8, vm, streams test benchmarks Aug 18, 2018

Trott commented Aug 18, 2018

Copy link
Copy Markdown
Member

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

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

Trott commented Aug 18, 2018

Copy link
Copy Markdown
Member

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

Trott commented Aug 19, 2018

Copy link
Copy Markdown
Member

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

This was referenced Aug 20, 2018
Closed
Closed

addaleax commented Aug 24, 2018

Copy link
Copy Markdown
Member

Landed in d495e40...902fd40

addaleax closed this Aug 24, 2018
addaleax pushed a commit that referenced this pull request Aug 24, 2018
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
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
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 deleted the add-more-test-benchmarks branch August 25, 2018 19:14
addaleax pushed a commit that referenced this pull request Aug 27, 2018
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
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
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
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
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
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 mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
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
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR left review comments
jasnell jasnell approved these changes
Trott Trott approved these changes
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

Back | FazBrowse Home | New Git URL