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 test for timers benchmarks #12851

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, benchmark

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 5, 2017
const path = require('path');

const runjs = path.join(__dirname, '..', '..', 'benchmark', 'run.js');
const argv = ['--set', 'thousands=0.001',
Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the thousands and millions in the configuration names because that way it would be easier to read the benchmark results.

@joyeecheung joyeecheung added the benchmark Issues and PRs related to the benchmark subsystem. label May 5, 2017
@joyeecheung
Copy link
Member Author

@@ -0,0 +1,21 @@
'use strict';

const common = require('../common');
Copy link
Contributor

Choose a reason for hiding this comment

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

Linter: 3:7 error 'common' is assigned a value but never used no-unused-vars

Copy link
Member Author

Choose a reason for hiding this comment

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

@vsemozhetbyt Oh no, my editor failed me :( Thanks for catching that!

@joyeecheung
Copy link
Member Author

Fixed the unsed var...New CI: https://ci.nodejs.org/job/node-test-pull-request/7890/

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

@joyeecheung
Copy link
Member Author

Landed in 771568a, thanks!

@joyeecheung joyeecheung closed this May 9, 2017
joyeecheung added a commit that referenced this pull request May 9, 2017
PR-URL: #12851
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12851
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

Should this be backported to v6.x? (I don't see why not is a reasonable answer!)

@joyeecheung
Copy link
Member Author

@gibfahn benchmark/run.js does not exist in 6.x, so marked as dont land

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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants