test: add test for child_process benchmark#12326
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added benchmark 
joyeecheung commented Apr 11, 2017
There was a problem hiding this comment.
I'm not sure about this change. In the other benchmarks, len is a message length in bytes I think. Here, it is a number of iterations, so len is kind of misleading because it's not a length. We tend to use n for this sort of thing instead.
I'm not opposed to this, but would be curious what others thought. @nodejs/benchmarking @mscdex
There was a problem hiding this comment.
Actually, I see that you already set n to 1 when you launch the benchmarks, so maybe this should just be n and that's that?
There was a problem hiding this comment.
Yes, we should be more consistent and use n where possible. There are some benchmarks like this and others that use thousands or millions though. I prefer to just use n, even if the values are large, for consistency.
There was a problem hiding this comment.
Yeah I think I renamed this to len because I mistook this one for another benchmark when editing :S..this should've been n, good catch!
joyeecheung commented Apr 13, 2017
Updated to use New CI: https://ci.nodejs.org/job/node-test-pull-request/7368/ |
joyeecheung commented Apr 17, 2017
I am going to land this in 24 hours if no one objects to the update :) |
joyeecheung commented Apr 19, 2017
Landed in 3d7c82b, thanks! |
PR-URL: #12326 Ref: #12068 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
vsemozhetbyt commented Apr 19, 2017
FWIW, |
jasnell commented Apr 19, 2017
Oh good, it's not just me. |
Trott commented Apr 19, 2017
This landed despite the added test failing on the Windows CI run for this PR. :-( Seems to run OK on all the CI Windows types except Windows 2016. Maybe |
Trott commented Apr 19, 2017
Although I don't think it would hang if that's the case...it would exit with ENOENT, I think. |
joyeecheung commented Apr 21, 2017
Oh no, sorry for the trouble, should've checked the new CI :( |
PR-URL: #12326 Ref: #12068 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
PR-URL: #12326 Ref: #12068 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
PR-URL: #12326 Ref: #12068 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
gibfahn added baking-for-lts 
MylesBorins removed the baking-for-lts 10 participants
Refs: #12068
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test, benchmark