test(NODE-6756): add tags to benchmarks #751
Conversation
W-A-James
changed the title
nbbeeken
added
the
Primary Review
test/bench/custom/readme.md
Outdated
| @@ -28,9 +32,9 @@ The JSON emitted at the end of the benchmarks must follow our performance tracki | |||
| The JSON must be an array of "`Test`"s: | |||
|
|
|||
| ```ts | |||
| type Metric = { name: string, value: number } | |||
| type Metric = { name: string, value: number, metadata: { improvement_diraction: 'up' | 'down' } } | |||
There was a problem hiding this comment.
| type Metric = { name: string, value: number, metadata: { improvement_diraction: 'up' | 'down' } } | |
| type Metric = { name: string, value: number, metadata: { improvement_direction: 'up' | 'down' } } |
test/bench/spec/bsonBench.ts
Outdated
| .run() | ||
| .then(() => { | ||
| return readFile(join(__dirname, '..', '..', 'etc', 'cpuBaseline.json'), 'utf8'); | ||
| }, console.error) |
There was a problem hiding this comment.
The console.error's are risky no? don't we want to crash?
test/bench/spec/bsonBench.ts
Outdated
| return readFile(join(__dirname, '..', '..', 'etc', 'cpuBaseline.json'), 'utf8'); | ||
| }, console.error) | ||
| .then(cpuBaseline => { | ||
| if (!cpuBaseline) throw new Error('could not find cpu baseline'); |
There was a problem hiding this comment.
Ah related to the other comment, if we just let the error throw then we'll know why it's missing maybe? (probably file not found, but who knows!)
test/bench/spec/bsonBench.ts
Outdated
| const rv = { ...result }; | ||
| rv.metrics = rv.metrics.filter(metric => metric.type === 'MEAN'); | ||
| return rv; | ||
| }); | ||
|
|
||
| const metadata: Metadata = { improvement_direction: 'up' }; | ||
| // calculte BSONBench composite score |
There was a problem hiding this comment.
| // calculte BSONBench composite score | |
| // calculate BSONBench composite score |
test/bench/spec/bsonBench.ts
Outdated
| suite | ||
| .run() | ||
| .then(() => { | ||
| return readFile(join(__dirname, '..', '..', 'etc', 'cpuBaseline.json'), 'utf8'); |
There was a problem hiding this comment.
Can you move this into the block below, make it an async () => function and await this? then stack traces are a bit nicer, nothing wrong with an async function chained to a promise.
| @@ -28,6 +28,9 @@ const DOCUMENT_ROOT = path.resolve(`${__dirname}/../documents`); | |||
| .run() | |||
| .catch(() => null); | |||
|
|
|||
| // Check for benchmark results | |||
| const cpuBaselineData = require(`${__dirname}${path.sep}cpuBaseline.json`); | |||
There was a problem hiding this comment.
require is already script relative, so "./cpuBaseline.json" should be all that you need
But also, if you have the path module, might as well use path.join() there may be additional rules to "joining" that that methods handles that are more than putting the .sep in place.
test/bench/custom/main.mjs
Outdated
| if (++completedSuites >= collectedSuites.length) { | ||
| let cpuBaselineResults; | ||
| try { | ||
| cpuBaselineResults = JSON.parse(fs.readFileSync(`${__dirname}/../etc/cpuBaseline.json`)); |
There was a problem hiding this comment.
Optional, should be able to use await import here to get the same convenience require provides in the other benchmark. This works so no worries if you want to stay the course.
|
The granular benchmarks timed out after 15min, it says the base commit takes 5min which I find a bit surprising, did we accidentally change iterations here? and should we do it intentionally later for a larger sample size? 😅 |
Description
What is changing?
getTypeTestTagsandgetMixedTestTagshelperIs there new documentation needed for these changes?
What is the motivation for this change?
Release Highlight
Fill in title or leave empty for no highlight
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript