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

benchmark: don't check wrk when testing non-http benchmark #1368

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

when testing non-http benchmark, no need the wrk tool.
so move the wrk check into http method.

@mscdex mscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Apr 8, 2015
@@ -121,7 +120,7 @@ Benchmark.prototype.http = function(p, args, cb) {

if (code) {
console.error('wrk failed with ' + code);
process.exit(code)
process.exit(code);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, not sure if we care about style here or not but I'd revert this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted.

@Fishrock123
Copy link
Contributor

LGTM otherwise

@Fishrock123
Copy link
Contributor

Also, @JacksonTian I'm assuming it may be intentional, but the email you use with git isn't registered to you on github, so the commits don't show up under your user. :)

@JacksonTian
Copy link
Contributor Author

it doesn't matter. That my company email.

发自我的 iPhone

在 2015年4月9日,上午1:00,Jeremiah Senkpiel notifications@github.com 写道:

Also, @JacksonTian I'm assuming it may be intentional, but the email you use with git isn't registered to you on github, so the commits don't show up under your user. :)


Reply to this email directly or view it on GitHub.

when testing non-http benchmark, no need the wrk tool.
so move the wrk check into http method.
bnoordhuis pushed a commit that referenced this pull request Apr 9, 2015
When running a non-http benchmark, there is no need the check for the
wrk tool so move the wrk check into the http method.

PR-URL: #1368
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@bnoordhuis
Copy link
Member

Cheers @JacksonTian, landed in d2b62a4.

@bnoordhuis bnoordhuis closed this Apr 9, 2015
@rvagg rvagg mentioned this pull request Apr 11, 2015
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants