benchmark, test: test the HTTP benchmark with a dummy benchmarker

[joyeecheung]

This is a retake of #12025, it basically just goes with the last option in #12025 (comment), makes a dummy http benchmarker to connect to the benchmarked http server once, and sets all the numeric benchmark configurations to the minimal.

• The first commit also refactors _http-benchmarks a bit to use ES6 classes.
• The second commit also moves http/_http_simple.js to fixtures/simple-http-server.js since it is not a benchmark itself

This adds about 10s to the test running time on my mac (i7-4770HQ CPU @ 2.20GHz) (as comparison, test-benchmark-net.js runs for about 5s)

Refs: #12068

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

[joyeecheung]

/cc @Trott @nodejs/benchmarking @nodejs/build

CI: https://ci.nodejs.org/job/node-test-pull-request/7089/

[joyeecheung]

It's timing out on pi1-raspbian-wheezy..should we skip this test on these slower devices, or set a larger timeout for them? (not sure how to do this)

[joyeecheung]

There is also another way to fix the timeout: test the benchmarks one by one in multiple tests using --filter of benchmark/run.js instead of checking them all in one test.

[Trott]

Love it! I'd be OK with "fixing" the Pi timing out problem by not running this test on Pi. Since it's just the Pi 1 that is failing you can do this:

if (!common.enoughTestMem) {
  common.skip('Insufficient memory for HTTP benchmark test');
  return;
}

[jasnell]

not intending to be pedantic... but can we use _noop-benchmarker (noop for non-operational) instead of _dummy-....

[Trott]

I don't like noop because noop means "no operator". A noop should do nothing whatsoever. But that's not the case here.

In this case, this is a test double, so _test-double-benchmarker maybe? Sometimes people call certain types of test doubles mocks and other types stubs, so mock-benchmarker or stub-benchmarker might also be appropriate. (I stick to test double so that I don't have to worry about whether or not something is a mock or a stub.)

[Trott]

Also, I'm fine with whatever name because this is an internal thing and we can always change it later. So if @jasnell or someone else disagrees with my suggestions,
¯\(ツ)/¯.

[joyeecheung]

Yeah it's not really a noop since it does connects to the server (a real noop-benchmarker would just skip the http.get call and write to stdout straight away). I think _test-double-benchmarker is better :D

[jasnell]

why change these?

[joyeecheung]

So test-benchmark-http.js can set the parameters for it (otherwise it will need to add --set num=1 --set size=1...also I think it's better to keep the configuration names consistent)

[joyeecheung]

Also if we want to do #12068, doing this to other benchmarks too will make it eaiser..

[joyeecheung]

Renamed dummy-benchmarker to test-trouble-benchmarker and skip test on Pi 1..PTAL, thanks @jasnell @Trott

New CI: https://ci.nodejs.org/job/node-test-pull-request/7150/

[joyeecheung]

node-test-commit-osx took 2h30m to run..although it doesn' seem to be related to this PR since https://ci.nodejs.org/job/node-test-commit-osx/8734/ and https://ci.nodejs.org/job/node-test-commit/8819/ took more than 2 hours too?

[joyeecheung]

Launch a osx CI against this one: https://ci.nodejs.org/job/node-test-commit-osx/8737/
and against master: https://ci.nodejs.org/job/node-test-commit-osx/8736/

[Trott]

I think you meant to rename it test-double/TestDouble rather than test-trouble/testTrouble...

[joyeecheung]

It's double now, thanks for catching that @vsemozhetbyt @Trott :D
new CI: https://ci.nodejs.org/job/node-test-pull-request/7152/

[Trott]

LGTM if CI is green

[Trott]

@jasnell Is @joyeecheung's answer to your question "why change the config property names" sufficient? If so can you update your review so this can be unblocked?

@joyeecheung Can you rebase?

(I'd really like to see this land. :-D )

[jasnell]

Yes. It's fine. I can't clear my review from the mobile view. This lgtm

No longer accurate

[joyeecheung]

Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/7277/

[Trott]

CI failures are unrelated. LGTM.

[joyeecheung]

Landed in c095394...2d3d4cc, thanks!

[italoacasas]

cc @joyeecheung

[joyeecheung]

Some of the changes depend on #10558, but should still be backportable. Doing a backport now...

[gibfahn]

@joyeecheung do you think it makes sense to backport this to Node 6?

If it is backported it should land with #13109 and #13390

[joyeecheung]

@gibfahn The new benchmark suite does not exist in 6.x so it can not be backported.