test: added fs benchmark #16049
test: added fs benchmark #16049
Conversation
nodejs-github-bot
added
benchmark
ctdio
force-pushed
the
add-test-benchmark-fs
branch
from
a404e56 to
77bfcd2
Compare
Trott
added
the
code-and-learn
test/parallel/test-benchmark-fs.js
Outdated
| "size=1", | ||
| "dur=1", | ||
| "filesize=1024" | ||
| ]); |
There was a problem hiding this comment.
Is the CI passing here? I would expect it to fail for e.g. benchmark/fs/bench-realpath.js as it requires a type that is not set here.
There was a problem hiding this comment.
The benchmark will run with the default values given to common.createBenchmark for any argument that is not specified. So in the case of benchmark/fs/bench-realpath.js, both the relative and resolved types are being run. I figured that since the purpose of pulling these benchmarks into the test suite was to ensure they don't break, it would be good to go through each of the code paths.
There was a problem hiding this comment.
Hello @charlieduong94,
What you say is true, and ideally that is what we would want, but that make this a very long test, and we are trying to strike a balance between coverage and manageability of the test suite.
Running this test on our fastest machine took 10 seconds (ignore the _ms in the log)
ok 242 parallel/test-benchmark-fs
---
duration_ms: 10.107
...
I'm assuming that if we run this on only a single type it should take ~33% of that time. (Personally I'd pick buf)
Anyway this is a good PR, so thank you very much.
There was a problem hiding this comment.
I can make that change if you would like. Some minor changes will need to be made to the read-stream-throughput benchmark to get the type arg to play nicely with each other.
|
Weclome @charlieduong94 and thank you for the contribution 🥇 Quick CI: https://ci.nodejs.org/job/node-test-commit-linuxone/9059 ✔️ |
ctdio
force-pushed
the
add-test-benchmark-fs
branch
from
77bfcd2 to
720abb0
Compare
| size: [1024, 4096, 65535, 1024 * 1024] | ||
| }); | ||
|
|
||
| function main(conf) { | ||
| type = conf.type; | ||
| size = +conf.size; | ||
| filesize = conf.filesize |
There was a problem hiding this comment.
Linter shows this line is missing a semicolon.
not ok 5 - /usr/home/iojs/build/workspace/node-test-linter/benchmark/fs/read-stream-throughput.js
---
message: Missing semicolon.
severity: error
data:
line: 21
column: 27
ruleId: semi
...
There was a problem hiding this comment.
Sorry, just pushed the fix
Modified fs benchmarks to use more unique args to avoid collision.
ctdio
force-pushed
the
add-test-benchmark-fs
branch
from
d62e0c1 to
1c173ae
Compare
Modified fs benchmarks to use more unique args to avoid collision. PR-URL: nodejs#16049 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
|
Landed in 58d26e2. |
Modified fs benchmarks to use more unique args to avoid collision. PR-URL: nodejs/node#16049 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Modified fs benchmarks to use more unique args to avoid collision. PR-URL: #16049 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
|
This does not land cleanly in v6.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported |
Had to make the
filesizeconfigurable for theread-stream-throughputbenchmark since the default was quite large and ended up causing the test to timeout.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes