Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(benchmark-reporter): Refactor MochaReporter to handle benchmark mocha test results #21730

Merged
merged 11 commits into from
Jul 15, 2024
Merged

refactor(benchmark-reporter): Refactor MochaReporter to handle benchmark mocha test results #21730

merged 11 commits into from
Jul 15, 2024

Conversation

chentong7
Copy link
Contributor

@chentong7 chentong7 commented Jul 1, 2024 •
edited
Loading

Description

This is a change to remove MochaTestMomeryReporter and only keep the general Mocha benchmark test reporter.

Sample Output

console output of using MochaReporter vs MochaTestMomeryReporter

Console log of runtime benchmark mocha test

console_output_mocha

Console log of memory benchmark mocha test

console_output_memory

Report of runtime benchmark mocha test

mocha_test_compare

Report of memory benchmark mocha test

memory_test_compare
@chentong7 chentong7 requested a review from a team as a code owner July 1, 2024 18:25
@github-actions github-actions bot added dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Jul 1, 2024
@chentong7 chentong7 changed the title Cleanup Jul 2, 2024
@chentong7 chentong7 requested a review from alexvy86 July 2, 2024 18:19
alexvy86
alexvy86 reviewed Jul 2, 2024
View reviewed changes
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't finish looking at all of it in detail but I think the comments below suggest a different direction for the PR.
pnpm-lock.yaml Outdated Show resolved Hide resolved
tools/benchmark/src/runBenchmark.ts Outdated Show resolved Hide resolved
tools/benchmark/src/ReporterUtilities.ts Outdated Show resolved Hide resolved
packages/dds/matrix/src/test/memory/.mocharc.cjs Outdated Show resolved Hide resolved
tools/benchmark/src/Reporter.ts Outdated Show resolved Hide resolved
tools/benchmark/src/Reporter.ts Outdated Show resolved Hide resolved
@chentong7 chentong7 requested a review from a team as a code owner July 9, 2024 00:55
@github-actions github-actions bot added area: dds Issues related to distributed data structures public api change Changes to a public API labels Jul 9, 2024
Josmithr
Josmithr reviewed Jul 9, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the area: dds Issues related to distributed data structures label Jul 9, 2024
@chentong7 chentong7 force-pushed the cleanup branch 3 times, most recently from b82b769 to ca6fce1 Compare July 9, 2024 21:52
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Jul 9, 2024
@chentong7 chentong7 requested review from Josmithr and alexvy86 and removed request for a team July 9, 2024 22:08
alexvy86
alexvy86 reviewed Jul 10, 2024
View reviewed changes
tools/benchmark/src/MochaReporter.ts Outdated Show resolved Hide resolved
tools/benchmark/src/utilities.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jul 11, 2024
alexvy86
alexvy86 reviewed Jul 11, 2024
View reviewed changes
packages/dds/matrix/src/test/memory/.mocharc.cjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
tools/benchmark/.eslintrc.cjs Outdated Show resolved Hide resolved
tools/benchmark/src/utilities.ts Outdated Show resolved Hide resolved
tools/benchmark/src/Reporter.ts Outdated Show resolved Hide resolved
tools/benchmark/src/index.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added the area: tests Tests to add, test infrastructure improvements, etc label Jul 12, 2024
alexvy86
alexvy86 reviewed Jul 12, 2024
View reviewed changes
.changeset/flat-moose-sell.md Outdated Show resolved Hide resolved
packages/dds/matrix/src/test/memory/.mocharc.cjs Outdated Show resolved Hide resolved
tools/benchmark/src/mocha/mochaReporterUtilities.ts Outdated Show resolved Hide resolved
tools/benchmark/src/test/ReporterUtilities.spec.ts Outdated Show resolved Hide resolved
tools/benchmark/src/runBenchmark.ts Outdated Show resolved Hide resolved
tools/benchmark/src/runBenchmark.ts Outdated Show resolved Hide resolved
tools/benchmark/src/resultFormatting.ts Outdated Show resolved Hide resolved
tools/benchmark/src/index.ts Show resolved Hide resolved
@chentong7 chentong7 requested a review from a team as a code owner July 12, 2024 17:02
alexvy86
alexvy86 reviewed Jul 12, 2024
View reviewed changes
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last batch of comments below, but we're almost there! :)
tools/benchmark/src/mocha/memoryTestRunner.ts Outdated Show resolved Hide resolved
tools/benchmark/src/mocha/memoryTestRunner.ts Outdated Show resolved Hide resolved
tools/benchmark/src/mocha/memoryTestRunner.ts Show resolved Hide resolved
tools/benchmark/src/index.ts Show resolved Hide resolved
tools/benchmark/CHANGELOG.md Outdated Show resolved Hide resolved
.changeset/flat-moose-sell.md Outdated Show resolved Hide resolved
experimental/dds/attributable-map/src/test/memory/.mocharc.cjs Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Jul 12, 2024
@github-actions github-actions bot removed the area: tests Tests to add, test infrastructure improvements, etc label Jul 12, 2024
alexvy86
alexvy86 approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀
@chentong7 chentong7 enabled auto-merge (squash) July 15, 2024 23:40
Josmithr
Josmithr approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the API-level changes, given the existing PR approval (I didn't look at everything in depth, just the API changes). Thanks for adding the changelog notes!
@chentong7 chentong7 merged commit 957b194 into microsoft:main Jul 15, 2024
28 checks passed
@chentong7 chentong7 deleted the cleanup branch July 15, 2024 23:49
@chentong7 chentong7 mentioned this pull request Jul 16, 2024
Merged
RishhiB pushed a commit to RishhiB/FluidFramework-1 that referenced this pull request Jul 18, 2024
@chentong7 @RishhiB
refactor(benchmark-reporter): Refactor MochaReporter to handle benchm…
1aff8a4 
…ark mocha test results (microsoft#21730)

## Description

This is a change to remove MochaTestMomeryReporter and only keep the
general Mocha benchmark test reporter.

## Sample Output

console output of using `MochaReporter` vs `MochaTestMomeryReporter`

### Console log of general benchmark mocha test

![console_output_mocha](https://github.com/microsoft/FluidFramework/assets/114451900/bd2ed735-e35a-4661-8217-3d57f7cfd00b)

### Console log of memory benchmark mocha test

![console_output_memory](https://github.com/microsoft/FluidFramework/assets/114451900/32245298-a3b5-4752-b452-eabda296ac35)


### Report of general benchmark mocha test

![mocha_test_compare](https://github.com/microsoft/FluidFramework/assets/114451900/6aa5be43-185b-42e0-a6f1-2c944648ac4b)


### Report of memory benchmark mocha test

![memory_test_compare](https://github.com/microsoft/FluidFramework/assets/114451900/bb339b71-5337-409d-bd8a-bb5aadede157)
jikim-msft
jikim-msft reviewed Jul 26, 2024
View reviewed changes
tools/benchmark/src/runBenchmark.ts
customData: {
"batch count": this.samples.length,
"period (ns/op)": stats.arithmeticMean,
"relative margin of error": stats.marginOfErrorPercent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change.
jikim-msft
jikim-msft reviewed Jul 26, 2024
View reviewed changes
tools/benchmark/src/runBenchmark.ts
"relative margin of error": stats.marginOfErrorPercent,
"iterations per batch": this.iterationsPerBatch,
},
customDataFormatters: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the keys in customDataFormatters to match those in customData.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch public api change Changes to a public API
5 participants
@chentong7 @alexvy86 @CraigMacomber @Josmithr @jikim-msft