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: add v8, vm, streams test benchmarks #22335

Closed
wants to merge 3 commits into from

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Aug 15, 2018

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.

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

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 changed the title Add v8,vm test benchmarks Add v8, vm, streams test benchmarks Aug 15, 2018
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

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

@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

@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

@addaleax
Copy link
Member

Landed in d495e40...902fd40

@addaleax addaleax closed this Aug 24, 2018
addaleax pushed a commit that referenced this pull request Aug 24, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 24, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 24, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Refs: #12068

PR-URL: #22335
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants