Skip to content

Fix/format model size and tests - #149

Open
RahulHarihar wants to merge 3 commits into
karansinghgit:mainfrom
RahulHarihar:fix/format-model-size-and-tests
Open

RahulHarihar wants to merge 3 commits into
karansinghgit:mainfrom
RahulHarihar:fix/format-model-size-and-tests

Conversation

@RahulHarihar

@RahulHarihar RahulHarihar commented Sep 18, 2026 •

Copy link
Copy Markdown

Summary

  • Fixes inconsistent decimal threshold (mb >= 1000) in formatModelSize by updating it to binary (mb >= 1024), matching the mb / 1024 divisor and formatBytes convention.
    • Note: This is a latent bug that changes nothing visible in the UI today, since every model size in the current catalog is outside the 1000 to 1023 MB range.
  • Adds unit tests for formatModelSize boundary values (999, 1000, 1023, 1024, 1536, 2048).
  • Tests plural (0, 1, 2) and edge cases for countWords.
  • Tests formatCompact with literal assertions beside number formatters.
  • Tests formatDay with literal values against a frozen now, including the previously untested branch for dates over a week old in the current year.

Fixes #148

Tested On

  • Windows 11 (Automated checks: npm run typecheck, npm test, npx vite build)

@karansinghgit

Copy link
Copy Markdown
Owner

The one-line fix is right and I'd like to take it: formatModelSize should use 1024 for both the threshold and the divisor, the way formatBytes does right below it. Worth saying in the description that it changes nothing visible today, since every model size in the catalog is outside the 1000 to 1023 range. It's a latent bug, which is still worth fixing.

Keep these tests: the formatModelSize boundaries (999, 1000, 1023, 1024, 1536, 2048), plural for 0, 1 and 2, and the extra countWords cases.

Please drop or rewrite the rest. Several assert that the function equals a copy of its own body, for instance formatCompact(1200) compared against a fresh Intl.NumberFormat with the same options, and the same pattern for formatTime, formatDay and formatNumber. Those can't fail no matter what the function does. The file already shows the better way: formatRelative and startOfDay assert literals against a frozen now, and the JSDoc gives the expected output (1234 → "1.2K").

One real gap while you're in there: formatDay's branch for a date over a week old in the current year is still untested. That's the branch with the actual logic.

Small thing: formatCompact fits better beside the other number formatters than in "durations and sizes", and renaming the "relative times" block isn't needed.

Thanks for the write-up in #148, it was unusually clear.

@RahulHarihar

Copy link
Copy Markdown
Author

Thanks for the review and guidance @karansinghgit! I've updated the tests to drop the tautological assertions, added the test for formatDay's current-year branch with literal values against frozen now, moved formatCompact beside the number formatters, and clarified in the PR description that this is a latent bug with no visual change to current catalog models.

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.

fix(v2): inconsistent threshold in formatModelSize and missing unit test coverage for format utilities

2 participants