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

Enable linting for benchmarks #5517

Closed
wants to merge 2 commits into from
Closed

Enable linting for benchmarks #5517

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 2, 2016

Refactor benchmarks to conform with existing lint rules. (In the process, a few broken benchmarks were fixed, e.g. the one for domains.) Enable linting for benchmarks.

/cc @nodejs/benchmarking @mscdex

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Mar 2, 2016
@Trott
Copy link
Member Author

Trott commented Mar 2, 2016

@@ -15,7 +16,8 @@ var bench = common.createBenchmark(main, {

function main(conf) {
var iter = (conf.iter) * 100000;
var aliceBuffer = fs.readFileSync(__dirname + '/../fixtures/alice.html');
var aliceBuffer = fs.readFileSync(
path.resolve(__dirname, '../fixtures/alice.html'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't having the last parenthesis on the next line be more consistent (and IMHO better looking) with the changes made in benchmark/compare.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Will change and push.

@Trott
Copy link
Member Author

Trott commented Mar 2, 2016

Fixup commit pushed based on @mscdex feedback. PTAL.

@mscdex
Copy link
Contributor

mscdex commented Mar 2, 2016

LGTM if CI is still ok with it

@Trott
Copy link
Member Author

Trott commented Mar 2, 2016

@targos
Copy link
Member

targos commented Mar 2, 2016

LGTM

2 similar comments
@evanlucas
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

Looks like the PR may need a quick rebase and update before landing.

@Trott
Copy link
Member Author

Trott commented Mar 2, 2016

@jasnell Rebased!

jasnell pushed a commit that referenced this pull request Mar 3, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 3, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 3, 2016

Landed in 1bedeeb and 6d22003

@jasnell jasnell closed this Mar 3, 2016
@Fishrock123 Fishrock123 mentioned this pull request Mar 7, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
PR-URL: #5517
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@Trott this one too for benchmarking backport

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

Successfully merging this pull request may close these issues.

6 participants