Convert local setup to a runner #63
Conversation
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
3c7d75f to
3c0dfb0
Compare
wesleytodd
force-pushed
the
add-local-server
branch
from
596a16a to
8de4e78
Compare
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
3c0dfb0 to
bf79959
Compare
|
There are still quite a few gaps:
But @RafaelGSS are you alright with this direction so that we align with the other runner styles and features? |
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
Sorry delay, @wesleytodd, a lot of things are going on recently, as you might know.
To be honest, I was trying to say "ditch that abstraction entirely" - It's a personal preference, so I'm fine leaving it as internal packages. My feeling that this increases the contribution barrier and makes things overly complex.
Also, I do believe Wrk2 (at least) should be a global binary - making it available through this CLI will make things very complex as the binary needs different dependencies according to each supported environment. Leaving that open for the "sysadmin" seems a better choice to not make things more complex than it is nowadays.
(That's what we do for Node.js too)
wesleytodd
force-pushed
the
add-local-server-runner
branch
2 times, most recently
from
b5706bc to
7e872b6
Compare
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
7e872b6 to
5a5c185
Compare
|
Ping me when this is ready to review @wesleytodd |
|
Will do, just cleaning things up and will move it from draft when it is ready. |
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
5a5c185 to
3499db7
Compare
cli: add local-bench to exbf Output example > @expressjs/perf-wg@1.0.0 local-load > expf local-load Running autocannon load... Result Autocannon: 121610.67 Running wrk2 load... Result Wrk2: [ { percentile: 50, ms: 0.829 }, { percentile: 75, ms: 1.1 }, { percentile: 90, ms: 1.41 }, { percentile: 99, ms: 1.75 }, { percentile: 99.9, ms: 1.88 }, { percentile: 99.99, ms: 2 }, { percentile: 99.999, ms: 2.2 }, { percentile: 100, ms: 2.2 } ]
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
3499db7 to
e764ef6
Compare
wesleytodd
force-pushed
the
add-local-server-runner
branch
2 times, most recently
from
2870e11 to
ce4622a
Compare
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
ce4622a to
a61605e
Compare
|
|
||
| // TODO: I don't see docs on how to do this with wrk2 | ||
| if (opts.method || this.method) { | ||
| throw new Error('not yet supported'); |
There was a problem hiding this comment.
method and body did not show up in the readme and since I didn't get wrk2 building on my mac I just left these here for now. We should fix this before merging.
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
I think it's also missing a doc update.
|
|
||
| // TODO: I don't see docs on how to do this with wrk2 | ||
| if (opts.method || this.method) { | ||
| throw new Error('not yet supported'); |
wesleytodd
force-pushed
the
add-local-server-runner
branch
2 times, most recently
from
ed5fc12 to
7dcde24
Compare
wesleytodd
force-pushed
the
add-local-server-runner
branch
from
7dcde24 to
fefc9f0
Compare
GroophyLifefor
left a comment
GroophyLifefor
left a comment
There was a problem hiding this comment.
I can not runned in my windows machine but I would prefer to merge this and fix in a clean pr rather than direct commits to branch or requesting changes.
3 participants
Took a quick look at converting @RafaelGSS's local server/client into a runner. I didn't have time this morning to look at the wrk2/autocannon stuff, but can assuming that #62 was not trying to say "ditch that abstraction entirely", which maybe it is and then I would like to discuss why we should do that.