Repository navigation
Refactor slow running tests #20128
Description
Our test suite takes quite a while currently and we constantly add new tests. A couple of those run really slow and block other tests to run in the meanwhile. So I checked which ones currently take longer than one second on my machine and this is the list:
Path: parallel/test-async-wrap-pop-id-during-load
Path: parallel/test-buffer-constructor-node-modules-paths
Path: parallel/test-buffer-indexof
Path: parallel/test-child-process-exec-encoding
Path: parallel/test-child-process-exec-timeout
Path: parallel/test-child-process-spawnsync-input
Path: parallel/test-cli-eval
Path: parallel/test-cli-eval-event
Path: parallel/test-cli-node-options
Path: parallel/test-cli-node-options-disallowed
Path: parallel/test-cli-node-print-help
Path: parallel/test-cli-syntax
Path: parallel/test-cluster-basic
Path: parallel/test-cluster-bind-privileged-port
Path: parallel/test-cluster-bind-twice
Path: parallel/test-cluster-disconnect
Path: parallel/test-cluster-master-kill
Path: parallel/test-cluster-worker-no-exit
Path: parallel/test-crypto-fips
Path: parallel/test-domain-abort-on-uncaught
Path: parallel/test-domain-with-abort-on-uncaught-exception
Path: parallel/test-env-var-no-warnings
Path: parallel/test-error-reporting
Path: parallel/test-errors-systemerror
Path: parallel/test-eslint-alphabetize-errors
Path: parallel/test-eslint-buffer-constructor
Path: parallel/test-eslint-documented-errors
Path: parallel/test-fs-read-stream-fd-leak
Path: parallel/test-fs-readdir-stack-overflow
Path: parallel/test-fs-readfile
Path: parallel/test-http-full-response
Path: parallel/test-http-pipeline-requests-connection-leak
Path: parallel/test-http-set-timeout-server
Path: parallel/test-http2-server-startup
Path: parallel/test-https-close
Path: parallel/test-internal-module-wrap
Path: parallel/test-module-loading-globalpaths
Path: parallel/test-module-main-fail
Path: parallel/test-module-main-preserve-symlinks-fail
Path: parallel/test-npm-install
Path: parallel/test-pipe-file-to-http
Path: parallel/test-postmortem-metadata
Path: parallel/test-preload
Path: parallel/test-process-argv-0
Path: parallel/test-readline-keys
Path: parallel/test-stdio-pipe-access
Path: parallel/test-timers-unref-active
Path: parallel/test-tls-securepair-leak
Path: parallel/test-trace-events-fs-sync
Path: addons/async-hello-world/test
Path: addons/async-hello-world/test-makecallback
Path: addons/async-hello-world/test-makecallback-uncaught
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary
Path: addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex
Path: addons-napi/test_async/test
Path: addons-napi/test_async/test-async-hooks
Path: addons-napi/test_async/test-uncaught
Path: sequential/test-child-process-pass-fd
Path: sequential/test-debugger-debug-brk
Path: sequential/test-fs-readfile-tostring-fail
Path: sequential/test-inspector-async-hook-setup-at-signal
Path: sequential/test-inspector-enabled
Path: sequential/test-inspector-not-blocked-on-idle
Path: sequential/test-inspector-port-cluster
Path: sequential/test-inspector-port-zero
Path: sequential/test-net-response-size
Path: sequential/test-performance
The sequential ones are particularly bad. A couple of these were just improved by @apapirovski in #20125 ❤️.
I believe we should go through this list and try to refactor a couple of these to reduce the overall load.
Update: I removed all tests where we already know that they should not be changed. This is the case for e.g., all benchmark tests after #20125.
Activity
refack commentedon Apr 18, 2018
Made a checklist is people want to "claim" some.
apapirovski commentedon Apr 18, 2018
I can confidently say that
should not be changed. They test important edge case behaviour.
Also, these can't really be changed any further after my PR from today lands:
They each run many different benchmark files so there's a Node.js startup cost involved for each additional benchmark that exists.
BridgeAR commentedon Apr 18, 2018
@apapirovski I removed those from the lists above.
apapirovski commentedon Apr 18, 2018
The addons tests might also be hard to fix. There's a bootup cost involved and some of those don't do that much.
(Also... sorry that I started immediately poking at it... I really appreciate the fact that you put this together! ❤️)
shobhitchittora commentedon May 1, 2018
@BridgeAR I was looking at some of the tests. Can you share some general things to look at for optimizations ? How to benchmark the test and how to see where are the bottlenecks ?
Thanks! 😇
tigercosmos commentedon May 12, 2018
Hi, it's my first time come here. If I want to improve one of these tests, are there any document or wiki that I can follow?
BridgeAR commentedon May 12, 2018
@shobhitchittora I did not look at the tests before. It will always be individual for each test case how to improve it. One thing that you could look for are timeouts. Those should likely be minimized while it is important to make sure the test will still cover the same parts as before while also staying reliable.
Besides that it is good to check what takes most time in the individual test. That can be done by using e.g.
console.time('foo'). Be aware that this does not log anything if the test passes. So insert athrow new Error()afterconsole.timeEnd('foo')(or better: at the end of the test while the end of the test might be an async operation). You will then see the output ofconsole.time(and of course of the error). However, console.time is a very rudimentary thing and some times it might be better to use the V8 profiler or other tools.If the test is heavily CPU bound and that is the reason for it to be slow, there is probably little that can be done about it and if that is your finding, please report that back as well. Some times the test could be rewritten in a less CPU bound way while still covering the same parts but if that is not the case, we should just accept that the specific test is going to take the time it takes.
@tigercosmos we have a guide how to write tests. See https://github.com/nodejs/node/tree/master/doc/guides/writing-tests.md. Besides that there is no further documentation. The tests are written just as any regular application code and should be mostly straight forward since it is only Node.js core. There are some helper functions that come from the
commontest file. These have a own documentation as well: https://github.com/nodejs/node/tree/master/test/common/README.mdshobhitchittora commentedon May 12, 2018
Thanks as always @BridgeAR. 👍🏻
BridgeAR commentedon May 12, 2018
One of the biggest issues in our tests is that we often use sync code instead of running things in parallel.
17 remaining items
fesebuv commentedon Aug 2, 2018
Hello folks,
I just opened a pr that updates
test/common/benchmark.jstest file in order to reduce the timetest-benchmark-buffertest takes to run.#22090
Trott commentedon Aug 2, 2018
Refactoring individual tests is unlikely to have much of an effect on our overall test runtime. I'm not bothered by how long it takes to run our tests, but if others are, here's what I would see as a productive way forward:
pummelpass.pummeltests added to the nightly job that also runs theinternettests.I'd prefer this issue be closed as I think it seems to attract well-meaning-but-misguided work. Those efforts could be put to better use.
BridgeAR commentedon Aug 2, 2018
I thought I already closed this.