test: reduce test-debugger-heap-profiler duration
#54843
Conversation
nodejs-github-bot
added
needs-ci
|
For refrence, before this change, I was getting: $ time node test/parallel/test-debugger-heap-profiler.js
real 0.53s
user 0.11s
sys 0.08s
cpu 34% |
| @@ -23,11 +24,11 @@ const filename = tmpdir.resolve('node.heapsnapshot'); | |||
| await cli.waitForInitialBreak(); | |||
| await cli.waitForPrompt(); | |||
| await cli.command('takeHeapSnapshot()'); | |||
| JSON.parse(readFileSync(filename, 'utf8')); | |||
| assert.ok(fs.existsSync(filename)); | |||
There was a problem hiding this comment.
Does this change the test? The original test would've thrown an error if the file wasn't valid JSON.
There was a problem hiding this comment.
other tests are already covers that. i don't think this is needed.
There was a problem hiding this comment.
This test is used as a regression test for a write race condition so checking for valid JSON makes sense. See 16a9ab1.
There was a problem hiding this comment.
I see. It appears that this test timeouts pretty often. Any recommendation on how to avoid?
There was a problem hiding this comment.
You could still (maybe) reduce the operation count by not reading the file to a string. JSON.parse accepts a buffer as input.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54843 +/- ##
==========================================
+ Coverage 87.61% 87.62% +0.01%
==========================================
Files 651 651
Lines 183343 183343
Branches 35441 35450 +9
==========================================
+ Hits 160641 160661 +20
+ Misses 15958 15946 -12
+ Partials 6744 6736 -8 |
github-actions
bot
removed
the
request-ci
| @@ -23,11 +24,11 @@ const filename = tmpdir.resolve('node.heapsnapshot'); | |||
| await cli.waitForInitialBreak(); | |||
| await cli.waitForPrompt(); | |||
| await cli.command('takeHeapSnapshot()'); | |||
| JSON.parse(readFileSync(filename, 'utf8')); | |||
| assert.ok(fs.existsSync(filename)); | |||
There was a problem hiding this comment.
| assert.ok(fs.existsSync(filename)); | |
| JSON.parse(fs.readFileSync(filename)); |
This way, the JSON is still being parsed, but FS isn't reading the file to a string
There was a problem hiding this comment.
It does not change anything. toString() is called on the input.
| // Check that two simultaneous snapshots don't step all over each other. | ||
| // Refs: https://github.com/nodejs/node/issues/39555 | ||
| await cli.command('takeHeapSnapshot(); takeHeapSnapshot()'); | ||
| JSON.parse(readFileSync(filename, 'utf8')); | ||
| assert.ok(fs.existsSync(filename)); |
Reduces the filesystem operations done on
test-debugger-heap-profilertest. I didn't understand why we calledJSON.parse(readfileSync)in the first place.