benchmark: add initial support for benchmark coverage #54333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
benchmark
|
Why don't we pass the function to benchmakr as a parameter to createBenchmark() like and later access it as or something which resolves into |
|
That requires changing all benchmarks and this is subject to developer errors. I tried to do it harmlessly (funny saying that while I'm monkey patching Module.require). But, possibly simpler than my approach. |
We can add an eslint rule (possibly @aduh95) to call that parameter inside |
This comment was marked as spam.
This comment was marked as spam.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey,
I'm opening it as a draft as there are a lot of loose ends to solve before considering such usage. This is an experiment related to research I'm conducting.
The idea is to generate a benchmark coverage for all of our exported modules. For instance, check if the functions exposed by
require('node:fs')are covered in our benchmark suite (benchmark/fs/*.js). To achieve an intermediary goal I had to monkey-patch theModule.prototype.requirebefore executing each benchmark (we run each one in a separate process) to return a singleton that basically manages the state of how many times that function was called.I have tried to use our
test_runnercoverage for that, but it couldn't identify the location of built-in modules (require('node:*')) which is expected by the nature of a coverage tool. The other reason it didn't fit was the need to cover only the exported modules, not thelib/internal/*functions. The result I wanted to have was: "I need to know which functions/classes that are exposed to users do not contain a benchmark".It's also worth it to mention that, a nested call is ignored in the benchmark report. Example:
fs.existscallsfs.accessbehind the scenesnode/lib/fs.js
Line 260 in b8a2550
fs.existsandfs.accessas "covered" in the benchmark report. It should only countfs.existsas covered.benchmark/**/*.jsResults
Full result: https://gist.github.com/RafaelGSS/66d33091560d61932b26d74af2fa8b82
Current limitations
coverage.jsto set up the monkey patch ofModule.require(-r ./coverage.js). However, any benchmark setup would impact the report. For instance:This will result in:
fs.existsSyncstatus "covered"fs.readdirstatus "covered"This is unfortunate, as in reality only
fs.existsSyncis being measured. One theory that came to my mind to fix that is to intercept thebench.startcalls and only then return the patched modules. I might try it later.typeof require('node:assert') === 'function'. This also means that classes such asAsyncLocalStorage(or function classes) aren't covered as the constructor (called withnew ..) isn't patched. I think usingProxymight solve it, but I haven't tested yet.I'm certain there's a better way to approach this goal and fix the limitations. Hence, I'm opening it as a draft.
cc: @nodejs/benchmarking @nodejs/test_runner @lemire