Skip to content

Factor out baseline calculation code from textreporter, for use in pretty and chart #98

Merged
RafaelGSS merged 3 commits intoRafaelGSS:mainfrom
jdmarshall:textReporter
Jul 22, 2025
Merged

Factor out baseline calculation code from textreporter, for use in pretty and chart #98
RafaelGSS merged 3 commits intoRafaelGSS:mainfrom
jdmarshall:textReporter

Conversation

@jdmarshall
Copy link
Copy Markdown
Collaborator

@jdmarshall jdmarshall commented Jul 20, 2025

I like your chart reports but humans can sometimes struggle a bit even with bar charts. So I wanted to append the faster/slower logic from textReporter.

Then I noticed some redundancy with pretty, and ended up here.

Before:

 ⇒ prom-client@latest                         | ██████████████████████████████ | 15,686,573 ops/sec | 11 samples
 ⇒ prom-client@trunk                          | █████████████████████████----- | 12,871,014 ops/sec | 12 samples
 ⇒ prom-client@current                        | ██████████████████████████---- | 13,426,771 ops/sec | 10 samples

After:

 ⇒ prom-client@latest                         | ██████████████████████████████ | 14,303,655 ops/sec | 12 samples (baseline)
 ⇒ prom-client@trunk                          | ██████████████████████████████ | 14,280,376 ops/sec | 10 samples (1.00x slower)
 ⇒ prom-client@current                        | ██████████████████████████████ | 14,343,253 ops/sec |  9 samples (1.00x faster)

Copy link
Copy Markdown
Owner

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

I liked the change on chart.js, but it seems the text.js is breaking. See full diff comparison on: https://gist.github.com/RafaelGSS/9b2e7d8d48d0a866a2cc283667ff9322

  • text reporter shouldn't have a \n between each one of them.
  • It seems you created the baseline analyzer, but you aren't using it on pretty reporter

Could you provide the screenshot when you change something? Otherwise it takes some time for me to checkout your PR, test the changes, and compare the result with main.

Comment thread lib/reporter/text.js Outdated
@jdmarshall jdmarshall force-pushed the textReporter branch 3 times, most recently from 2628b84 to 9c33c1b Compare July 21, 2025 15:16
@jdmarshall
Copy link
Copy Markdown
Collaborator Author

I wonder what the difference in settings is between prom-client and bench-node, because Webstorm lights up with every formatting issue with the former and I'm getting no warnings locally from the latter.

Feature parity for textReporter, but chartReporter doesn't work this way.
@jdmarshall jdmarshall force-pushed the textReporter branch 4 times, most recently from f2146e6 to b42f3b7 Compare July 21, 2025 20:48
@jdmarshall
Copy link
Copy Markdown
Collaborator Author

Finally got past the linter. Woof.

@RafaelGSS
Copy link
Copy Markdown
Owner

@jdmarshall, would you mind providing the graphic difference between the main and this PR? I mean, what changes in the screen (for pretty, text, and chart report)? This would help people when looking at the CHANGELOG and ending up on this PR.

@jdmarshall
Copy link
Copy Markdown
Collaborator Author

In the PR or in the commit message?

@RafaelGSS RafaelGSS merged commit 133c156 into RafaelGSS:main Jul 22, 2025
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants