test: clarify toPercent boundary at 1.0#4436
test: clarify toPercent boundary at 1.0#4436ramonmatias19 wants to merge 1 commit intopaperclipai:masterfrom
Conversation
Greptile SummaryThis pure-test PR adds a single case asserting that a utilization value of
Confidence Score: 4/5Safe to merge only after the boundary test at 1.0 is added and the PR description is corrected to match the actual changes. Two P1 findings: the exact 1.0 boundary case promised by the title is untested, and the PR description describes changes (boundary-test split, 101 clamp-overshoot) that do not appear in the diff. Both should be addressed before merging. server/src/tests/quota-windows.test.ts — missing the 1.0 boundary assertion and the described clamp-overshoot case. Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: server/src/__tests__/quota-windows.test.ts
Line: 579-583
Comment:
**Boundary case at 1.0 missing despite PR title**
The PR title is "clarify toPercent boundary at 1.0" and the description states that `1.0` should become `1%` (not `100%`), but the new test only exercises `42` — well above the boundary. The exact boundary value `1.0` (the first integer value that triggers the "already-percentage" path instead of the fraction-multiplication path) is never asserted. Without a test at exactly `1.0`, the stated guarantee isn't verified and a future refactor could regress it silently.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: server/src/__tests__/quota-windows.test.ts
Line: 579-583
Comment:
**PR description describes changes not present in the diff**
The description says "Split the existing boundary test into two cases and add `101` to the clamp-overshoot test," but the diff only adds a single new test and neither a split of the existing boundary test nor a `101` clamp-overshoot test appears anywhere. Please either update the description to match the actual change, or include the missing test cases.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test: clarify toPercent boundary at 1.0" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("handles utilization as already-percentage (>= 1) instead of fraction", async () => { | ||
| mockFetch({ five_hour: { utilization: 42, resets_at: null } }); | ||
| const windows = await fetchClaudeQuota("token"); | ||
| expect(windows[0]!.usedPercent).toBe(42); | ||
| }); |
There was a problem hiding this comment.
The PR title is "clarify toPercent boundary at 1.0" and the description states that 1.0 should become 1% (not 100%), but the new test only exercises 42 — well above the boundary. The exact boundary value 1.0 (the first integer value that triggers the "already-percentage" path instead of the fraction-multiplication path) is never asserted. Without a test at exactly 1.0, the stated guarantee isn't verified and a future refactor could regress it silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/__tests__/quota-windows.test.ts
Line: 579-583
Comment:
**Boundary case at 1.0 missing despite PR title**
The PR title is "clarify toPercent boundary at 1.0" and the description states that `1.0` should become `1%` (not `100%`), but the new test only exercises `42` — well above the boundary. The exact boundary value `1.0` (the first integer value that triggers the "already-percentage" path instead of the fraction-multiplication path) is never asserted. Without a test at exactly `1.0`, the stated guarantee isn't verified and a future refactor could regress it silently.
How can I resolve this? If you propose a fix, please make it concise.| it("handles utilization as already-percentage (>= 1) instead of fraction", async () => { | ||
| mockFetch({ five_hour: { utilization: 42, resets_at: null } }); | ||
| const windows = await fetchClaudeQuota("token"); | ||
| expect(windows[0]!.usedPercent).toBe(42); | ||
| }); |
There was a problem hiding this comment.
The description says "Split the existing boundary test into two cases and add 101 to the clamp-overshoot test," but the diff only adds a single new test and neither a split of the existing boundary test nor a 101 clamp-overshoot test appears anywhere. Please either update the description to match the actual change, or include the missing test cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/__tests__/quota-windows.test.ts
Line: 579-583
Comment:
**PR description describes changes not present in the diff**
The description says "Split the existing boundary test into two cases and add `101` to the clamp-overshoot test," but the diff only adds a single new test and neither a split of the existing boundary test nor a `101` clamp-overshoot test appears anywhere. Please either update the description to match the actual change, or include the missing test cases.
How can I resolve this? If you propose a fix, please make it concise.
Make the < 1 fraction heuristic explicit: values >= 1 are treated as already-percentage, so 1.0 becomes 1% (not 100%). Split the existing boundary test into two cases and add 101 to the clamp-overshoot test.
ramonmatias19
force-pushed
the
feature/quota-test-clarifications
branch
from
e5395a2 to
7f0e70d
Compare
1 participant
Summary
Make the
< 1fraction heuristic explicit: values >= 1 are treated as already-percentage, so1.0becomes1%(not100%). Split the existing boundary test into two cases and add101to the clamp-overshoot test.Pure test change — no production code touched.
Test plan
pnpm -C server test— all quota-window tests pass