Merged
Conversation
- Activate Arabic tests for triple digits through billions with correct expected values - Rewrite Russian tests to cover all ranges (single through trillions) - Add Chinese tests for the hundred millions (亿) range - Add English tests for teen numbers (13-19 default case) - Add Portuguese tests for the trillions range (idx===4 branch) - Add stub coverage tests for all unimplemented languages (bn, de, es, fr, hi, ja, mr, sw, ur) - Add LRU cache eviction test, memoize=false test, and silent=false error test to factory - Add num-in-words.ts integration tests for en, id, and unsupported languages - Fix fragile experimental warning test that depended on test execution order Closes ImBIOS#2
|
@EmersonBraun is attempting to deploy a commit to the ImBIOS Hobby Projects Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request expands test coverage across multiple languages to achieve higher coverage thresholds. Key changes include activating previously stubbed Arabic and Russian language tests with experimental mode support, adding language coverage stub tests for unimplemented languages, introducing new test modules for the numInWords factory and top-level function, and updating method signatures to accept optional configuration parameters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
library/src/lang/en.test.ts (1)
27-29: Add irregular teen assertions alongside the new teen coverage.Line 27-Line 29 improves teen branch coverage, but it still misses irregular English teen spellings (
13,15,18), which are high-value regression checks. Consider adding them in this table.✅ Suggested test additions
test.each<readonly [number, string]>([ [10, 'ten'], [11, 'eleven'], [12, 'twelve'], + [13, 'thirteen'], [14, 'fourteen'], + [15, 'fifteen'], [17, 'seventeen'], + [18, 'eighteen'], [19, 'nineteen'], [21, 'twenty one'], [99, 'ninety nine'], ])('%i should return %s', (num, expected) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/lang/en.test.ts` around lines 27 - 29, Add assertions for the irregular English teen spellings by inserting entries for 13, 15, and 18 into the same test table used for teen coverage in library/src/lang/en.test.ts (the array that currently contains [14,'fourteen'], [17,'seventeen'], [19,'nineteen']). Specifically add [13,'thirteen'], [15,'fifteen'], and [18,'eighteen'] alongside the existing teen cases so the test verifies these irregular spellings as well.library/src/lang/zh.test.ts (1)
84-86: Edge-range test should include both boundaries, not just one sample.The title says “Line 85 gap range (10M-100M)”, but only
10,000,000is asserted. Consider adding an upper-end value (e.g.,99,999,999) so the full range contract is protected.Proposed test tightening
describe('Edge cases', () => { - test('should return "Not supported" for numbers in the gap range (10M-100M)', () => { - expect(chineseNumInWords(10000000)).toBe('Not supported'); - }); + test.each<readonly [number]>([ + [10000000], + [99999999], + ])('should return "Not supported" for %i in the gap range (10M-100M)', (num) => { + expect(chineseNumInWords(num)).toBe('Not supported'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/lang/zh.test.ts` around lines 84 - 86, The test for the gap range only asserts the lower boundary; update the Edge cases test for chineseNumInWords to assert both boundaries of the 10M–100M gap by adding an expectation for the upper-end value (e.g., 99_999_999) to also return 'Not supported' alongside the existing 10_000_000 check so the full range contract is covered in the test suite.library/src/lang/ar.test.ts (1)
101-109: Consider adding a clarifying comment for the test strategy.These edge case tests for teens (13-19) and round tens (20, 30) use
{ experimental: true }, while the "Double digits" section tests similar number ranges without the experimental flag. If this is intentional to exercise both code paths, a brief comment explaining the coverage strategy would help future maintainers understand why similar numbers are tested differently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/lang/ar.test.ts` around lines 101 - 109, Tests for teens and round tens call arabicNumInWords with { experimental: true } while other double-digit tests do not, which is confusing; add a brief clarifying comment above the test blocks (referencing the test cases using arabicNumInWords and the experimental flag) explaining that similar numbers are intentionally tested against both the experimental and default code paths to ensure coverage, e.g., annotate the "should handle round tens" and "should handle teens" blocks to state they exercise the experimental implementation and the "Double digits" block covers the non-experimental implementation.library/src/utils/num-in-words-factory.test.ts (1)
188-195: Consider asserting the thrown error message explicitly.Line 195 checks only that “something” throws; matching the expected message would make this test less permissive.
Proposed tightening
- ).toThrow(); + ).toThrow('is too large to be converted');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/utils/num-in-words-factory.test.ts` around lines 188 - 195, The test for numInWordsFactory currently only asserts that factory(...) throws; tighten it by expecting the specific error message so the test fails for unexpected errors: capture the thrown error from calling factory(Number.MAX_SAFE_INTEGER + 1, { silent: false }) and replace expect(...).toThrow() with an assertion that matches the exact message or a suitable regex (e.g., using toThrowError('...') or toThrowError(/.../)); reference the numInWordsFactory factory invocation in the test and assert the known error string returned by the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/lang/pt.test.ts`:
- Around line 121-129: Tests in the Trilhões suite assert inconsistent output
('um bilião', 'um um bilião um') that conflicts with the existing trillion
contract elsewhere; update the tests that call
portugueseNumInWords(1000000000000) and portugueseNumInWords(1000000000001) to
assert the behavior-focused language contract (e.g., use 'um trilhão' and
appropriate remainder wording per the rest of the suite) and remove the brittle
internal-logic title "triggers idx===4 branch"—rename it to a semantic
description like "handles trillion with remainder" and assert only the expected
natural-language strings produced by portugueseNumInWords.
In `@library/src/lang/ru.test.ts`:
- Around line 55-59: The expected Russian strings in ru.test.ts use incorrect
declensions/genders for thousands/millions/etc.; update the test expectations
for the listed tuples ([1000, 1001, 2001, 4321, 9999] and the other ranges
noted) to use correct Russian forms (e.g., 1000 -> "одна тысяча", 1001 -> "одна
тысяча один", 2001 -> "две тысячи один", 4321 -> "четыре тысячи триста двадцать
один", 9999 -> "девять тысяч девятьсот девяносто девять") and ensure similar
fixes for millions/milliards/trillions using proper plural forms
("миллион/миллиона/миллионов", "миллиард/миллиарда/миллиардов",
"триллион/триллиона/триллионов") so tests in ru.test.ts match grammatically
correct Russian inflections.
- Around line 4-5: Current tests route all calls through the experimental mode
by defining const ru = (num: number) => russianNumInWords(num, { experimental:
true }); — add coverage for the default implementation by keeping the
experimental helper but also adding a small smoke set that calls
russianNumInWords(num) with no options; update or add assertions similar to
existing tests to validate a few representative numbers (e.g., 0, 1, 21) using
russianNumInWords directly so regressions in the default path are caught while
preserving experimental-mode tests via the ru helper.
In `@library/src/num-in-words.test.ts`:
- Around line 5-9: The test currently calls errorSpy.mockReset() in afterEach
but does not restore the original console.error, causing possible cross-test
pollution; update the teardown for the spy created as errorSpy (spying on
console.error) to restore the original implementation after each test (use the
spy's mockRestore or equivalent) instead of or in addition to mockReset so the
console.error method is returned to its original function when the suite
finishes.
In `@library/src/utils/num-in-words-factory.test.ts`:
- Around line 163-174: The test currently only checks that the mock converter fn
was called at least once, which doesn't prove eviction; update the test using
numInWordsFactory, MAX_CACHE_SIZE and the mock fn so after filling the cache you
verify the oldest key is recomputed: fill the cache with keys from 20000 to
20000 + MAX_CACHE_SIZE + 1, then reset/clear the mock call history (e.g.,
fn.mockClear()), call factory(20000) again and assert that fn is invoked for
that call (e.g., expect(fn).toHaveBeenCalled()/toHaveBeenCalledTimes(1)),
demonstrating the original entry was evicted and recomputed.
---
Nitpick comments:
In `@library/src/lang/ar.test.ts`:
- Around line 101-109: Tests for teens and round tens call arabicNumInWords with
{ experimental: true } while other double-digit tests do not, which is
confusing; add a brief clarifying comment above the test blocks (referencing the
test cases using arabicNumInWords and the experimental flag) explaining that
similar numbers are intentionally tested against both the experimental and
default code paths to ensure coverage, e.g., annotate the "should handle round
tens" and "should handle teens" blocks to state they exercise the experimental
implementation and the "Double digits" block covers the non-experimental
implementation.
In `@library/src/lang/en.test.ts`:
- Around line 27-29: Add assertions for the irregular English teen spellings by
inserting entries for 13, 15, and 18 into the same test table used for teen
coverage in library/src/lang/en.test.ts (the array that currently contains
[14,'fourteen'], [17,'seventeen'], [19,'nineteen']). Specifically add
[13,'thirteen'], [15,'fifteen'], and [18,'eighteen'] alongside the existing teen
cases so the test verifies these irregular spellings as well.
In `@library/src/lang/zh.test.ts`:
- Around line 84-86: The test for the gap range only asserts the lower boundary;
update the Edge cases test for chineseNumInWords to assert both boundaries of
the 10M–100M gap by adding an expectation for the upper-end value (e.g.,
99_999_999) to also return 'Not supported' alongside the existing 10_000_000
check so the full range contract is covered in the test suite.
In `@library/src/utils/num-in-words-factory.test.ts`:
- Around line 188-195: The test for numInWordsFactory currently only asserts
that factory(...) throws; tighten it by expecting the specific error message so
the test fails for unexpected errors: capture the thrown error from calling
factory(Number.MAX_SAFE_INTEGER + 1, { silent: false }) and replace
expect(...).toThrow() with an assertion that matches the exact message or a
suitable regex (e.g., using toThrowError('...') or toThrowError(/.../));
reference the numInWordsFactory factory invocation in the test and assert the
known error string returned by the implementation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
library/src/lang/ar.test.tslibrary/src/lang/bn.test.tslibrary/src/lang/de.test.tslibrary/src/lang/en.test.tslibrary/src/lang/es.test.tslibrary/src/lang/fr.test.tslibrary/src/lang/hi.test.tslibrary/src/lang/ja.test.tslibrary/src/lang/mr.test.tslibrary/src/lang/pt.test.tslibrary/src/lang/ru.test.tslibrary/src/lang/sw.test.tslibrary/src/lang/ur.test.tslibrary/src/lang/zh.test.tslibrary/src/num-in-words.test.tslibrary/src/utils/num-in-words-factory.test.ts
- pt.test.ts: rename describe/test to behavior-focused names, remove brittle internal-logic references (idx===4) - ru.test.ts: add default mode smoke tests, add comment noting declension is an implementation concern - num-in-words.test.ts: add afterAll with mockRestore to prevent cross-suite console.error pollution - num-in-words-factory.test.ts: improve eviction test to verify oldest entry is recomputed and recent entry remains cached
EmersonBraun
force-pushed
the
feat/100-percent-coverage
branch
from
8da0dcb to
171430d
Compare
Fix the Russian implementation to use proper plural forms and feminine gender for thousands (одна тысяча, две тысячи) instead of incorrect base forms (один тысяча, два тысяча). Apply correct Russian plural rules for миллион/миллиона/миллионов, миллиард/миллиарда/миллиардов, and триллион/триллиона/триллионов.
ImBIOS
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #2 — Make the coverageThreshold to 100%.
This PR achieves 100% function coverage and 100% line coverage across all 22 source files in the library by adding and activating tests for previously uncovered code paths.
Changes
Language tests (activated/added):
describe.todowas preventing all tests from runningdefaultcase in the teens switchidx === 4branch in the loopexperimental: trueto exercise the stub implementationUtility tests:
MAX_CACHE_SIZE),memoize: falsetest, andsilent: falseerror throwing test. Fixed a fragile test that depended on test execution order (module-levelisWarnedflag)Coverage Report (before → after)
Notes
.todotests for unimplemented features remain as.todo— only tests for actually implemented code paths were activatedSummary by CodeRabbit
New Features
Tests