Skip to content

Conversation

@MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Jul 15, 2025

Description

Actually resolves #277. EDIT: or actually not sure if this solves the same issue. But solves an issue anyway.

Examples:

Incorrect label, saying "M" aka "Mega" (or I guess "Millions" also reads fine):
Incorrect unit label M denoting Mega

Correct label, saying "K" aka "Kilo":
Correct unit label K denoting Kilo

To be completely clear, both these images are 100% correct other than the first image mentioning "M" aka 10^6.

Feel free to review on your own schedule.

How Has This Been Tested?

Manually in my PRs.

Minimal checklist:

  • I have performed a self-review of my own code
  • I have added DocC code-level documentation for any public interfaces exported by the package
  • I have added unit and/or integration tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Jul 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.41%. Comparing base (5c3a2ee) to head (d88a154).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   69.41%   69.41%   +0.01%     
==========================================
  Files          33       33              
  Lines        4053     4051       -2     
==========================================
- Hits         2813     2812       -1     
+ Misses       1240     1239       -1     
Files with missing lines Coverage Δ
Sources/Benchmark/BenchmarkResult.swift 62.93% <100.00%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
Sources/Benchmark/BenchmarkResult.swift 62.93% <100.00%> (+0.05%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c3a2ee...d88a154. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hassila
Copy link
Contributor

hassila commented Aug 25, 2025

Would you mind rebasing this on the latest main? I merged PR 1/4 with slight changes.

@MahdiBM MahdiBM force-pushed the mmbm-units-mismatch-fix branch from aac61b6 to 1b742a8 Compare August 25, 2025 17:11
@MahdiBM MahdiBM force-pushed the mmbm-units-mismatch-fix branch from 1b742a8 to d88a154 Compare August 25, 2025 17:16
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Aug 25, 2025

@hassila done

@hassila
Copy link
Contributor

hassila commented Aug 25, 2025

Do you have an example that generated the output from the screenshots? Would be good to understand the configuration used for the test.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Aug 25, 2025

@hassila not 100% sure, but try this:

Benchmark.defaultConfiguration.units = [<metric>: .kilo]

and make sure the output should not be in kilos (but observe that it will be, without this PR).

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Aug 25, 2025

I just noticed you asked for an example not just the configuration, I'm already off the desk though so I'll grab that tomorow.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Aug 26, 2025

This is what i think was the benchmark when the issue here happened (although i think even the new benchmarks would have this issue):

https://github.com/MahdiBM/swift-dns/blob/202d4734bf158887a5691c2871bfeb8ecca7c73c/Benchmarks/Name/Name.swift#L160

I think you can just compare 2 strings instead of 2 Names, since that's a custom type.

@hassila
Copy link
Contributor

hassila commented Aug 26, 2025

Thanks, I've reproduced this and verified the fix with:

swift package --allow-writing-to-package-directory benchmark thresholds update --target Basic --filter "Equality_Check_Identical_Throughput"
swift package benchmark thresholds check --target Basic --filter "Equality_Check_Identical_Throughput"

(just a note for the future)

@hassila hassila merged commit 6b8c88f into ordo-one:main Aug 26, 2025
15 of 17 checks passed
@hassila
Copy link
Contributor

hassila commented Aug 26, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Time is not scaled with scaling factor, but the result is

2 participants