-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: create benchmark test for misc and module #16044
test: create benchmark test for misc and module #16044
Conversation
'millions=.000001', | ||
'type=extend', | ||
'concat=0' | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will likely result in an error as e.g. misc/console.js
requires a method and none is set here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I provide a method in the file it will also apply to other benchmarks that use the same parameter, but don't implement the same functions, eg object-property-bench.js
.
Conversly, by not defining it the benchmarks use their default values.
Path: parallel/test-benchmark-misc
misc/console.js
misc/console.js n=1 concat=0 method="restAndSpread": 577.2662172841588
misc/freelist.js
misc/freelist.js n=1: 1,740.3744241536124
misc/function_call
misc/object-property-bench.js
misc/function_call.js Binding failed to load
/Users/charles.wall.3/projects/node/benchmark/misc/object-property-bench.js:79
throw new Error('Unexpected method');
^
Error: Unexpected method
at main (/Users/charles.wall.3/projects/node/benchmark/misc/object-property-bench.js:79:13)
at Benchmark.process.nextTick (/Users/charles.wall.3/projects/node/benchmark/common.js:34:28)
at _combinedTickCallback (internal/process/next_tick.js:131:7)
at process._tickCallback (internal/process/next_tick.js:180:9)
at Function.Module.runMain (module.js:643:11)
at startup (bootstrap_node.js:187:16)
at bootstrap_node.js:605:3
assert.js:45
throw new errors.AssertionError({
^
AssertionError [ERR_ASSERTION]: 1 === 0
at ChildProcess.child.on (/Users/charles.wall.3/projects/node/test/common/benchmark.js:25:12)
at emitTwo (events.js:125:13)
at ChildProcess.emit (events.js:213:7)
at Process.ChildProcess._handle.onexit (internal/child_process.js:210:12)
Command: out/Release/node /Users/charles.wall.3/projects/node/test/parallel/test-benchmark-misc.js
[02:13|% 100|+ 1971|- 1]: Done
make: *** [test] Error 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output above is after defining method to equal restAndSpread
which is not defined in the other benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am sorry, when I looked at this I forgot how the test worked again. Just ignore my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI passes.
'millions=.000001', | ||
'type=extend', | ||
'concat=0' | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am sorry, when I looked at this I forgot how the test worked again. Just ignore my comment.
Landed in be3ac44 Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 ! |
PR-URL: #16044 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #16044 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs/node#16044 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test