Add a unit-test harness (Vitest), typecheck script, and CI gates - #16514
Add a unit-test harness (Vitest), typecheck script, and CI gates#16514bodhish wants to merge 9 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds Vitest configuration and scripts, tests decimal, monetary, and medication logic, and updates CI with Node 24, npm caching, typechecking, and unit-test gates. ChangesUnit testing and CI validation
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Deploying care-preview with
|
| Latest commit: |
506f3e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5b04323b.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://issues-16507-vitest-typechec.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
Pull request overview
Bootstraps a backend-free verification baseline for CARE FE by introducing a minimal Vitest unit-test harness, a typecheck script (including generation of the gitignored supportedBrowsers artifact), and CI gating so PRs are blocked on both typechecking and unit tests.
Changes:
- Add
vitest.config.mtsand seed unit tests for the Decimal.js money helpers (src/Utils/decimal.test.ts). - Add
test:unit,test:unit:watch, andtypecheckscripts; addvitestas a devDependency. - Update
.github/workflows/linter.ymlto enable npm caching and to runtypecheck+ unit tests in CI.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.mts | Adds Vitest configuration with repo path aliases and Node as the default environment. |
| src/Utils/decimal.test.ts | Adds an initial unit-test suite covering Decimal-based money helper behavior. |
| package.json | Introduces unit-test/typecheck scripts and adds Vitest dependency. |
| package-lock.json | Locks new Vitest dependency tree. |
| .github/workflows/linter.yml | Adds npm cache plus typecheck and unit-test CI gates. |
| it("roundUp always rounds toward positive infinity to whole units", () => { | ||
| expect(roundUp("10.01")).toBe("11"); | ||
| }); |
Greptile SummaryThis PR bootstraps Vitest as the unit-test harness, adds
Confidence Score: 5/5Safe to merge — the infrastructure changes are additive and the three substantive fixes (double-read body, incomplete cache clearing, missing sign-out cleanup) are all correctness improvements backed by new tests. All changed code paths are well-tested by the new suite, the CI gate additions are straightforward, and the behavior changes in queryClient/AuthUserProvider are clear improvements over the previous implementation. No regressions were identified. No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "Clear persisted query cache on logout (#..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef8c5ef4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,26 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
With this file under src/ as *.test.ts, the existing npm run unimported step in .github/workflows/linter.yml will flag it as an unimported file: .unimportedrc.json only starts from app/script entries and does not include or ignore Vitest tests, while the unimported docs state that CI fails on unimported files not explicitly configured and its built-in test ignores are *.tests.*/*.spec.*, not singular *.test.*. Because this test is intentionally loaded only by Vitest, every PR/merge-group linter run will fail until tests are added to entry, ignorePatterns, or ignoreUnimported.
Useful? React with 👍 / 👎.
🎭 Playwright Test ResultsStatus: ✅ Passed
📊 Detailed results are available in the playwright-final-report artifact. Run: #10203 |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
|
@copilot resolve the merge conflicts in this pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/types/base/monetaryComponent/monetaryComponent.test.ts`:
- Around line 238-251: Update the test case around getPriceBreakdown so its
component amount requires rounding to two decimal places, such as 33.335 or
33.334, and adjust the expected breakdown values to assert the resulting rounded
amount multiplied by the quantity. Keep the existing quantity and breakdown
assertions otherwise unchanged.
- Around line 145-160: Update getDiscountAmount so its conditions filter treats
both undefined conditions and an explicitly empty array as applicable, using the
existing conditions-based filtering logic. Then revise the test case in
monetaryComponent.test.ts to expect the discount to be applied for a discount
without a conditions field, and remove the wording that pins the current
behavior as intentional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c61ac07-4350-4997-9ffc-8f53cb0edd8d
📒 Files selected for processing (2)
src/types/base/monetaryComponent/monetaryComponent.test.tssrc/types/emr/medicationRequest/medicationRequest.test.ts
| it("excludes discounts whose conditions field is undefined (pins current behavior: conditions must be an explicit empty array to apply — flagged for maintainer review)", () => { | ||
| // `c.conditions?.length === 0` is false when `conditions` is undefined | ||
| // (`undefined?.length === 0` evaluates to `undefined === 0` -> false), so a | ||
| // discount with no `conditions` key at all is silently excluded, the same | ||
| // as one with a populated conditions array. Pinned here as current | ||
| // behavior, not necessarily intended behavior. | ||
| const components: MonetaryComponent[] = [ | ||
| { monetary_component_type: MonetaryComponentType.base, amount: "100" }, | ||
| { | ||
| monetary_component_type: MonetaryComponentType.discount, | ||
| factor: "20", | ||
| // no `conditions` key | ||
| }, | ||
| ]; | ||
| expect(getDiscountAmount(components).toString()).toBe("0"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test pins a likely production bug in getDiscountAmount's conditions filter.
Per the provided contract, getDiscountAmount filters with c.conditions?.length === 0. When conditions is undefined, undefined?.length === 0 evaluates to undefined === 0 → false, so a discount with no conditions key is silently excluded — identical treatment to a discount with an actual, non-empty condition list. That's a real business-logic gap: discounts meant to always apply (no conditions field at all) would incorrectly not be applied, understating discounts / overcharging. The test comment itself flags this for maintainer review rather than asserting it's correct.
Since monetaryComponent.ts isn't in this diff, I can't fix it here, but recommend changing the production filter to !c.conditions?.length (treats both undefined and [] as "no conditions") and updating this test's expectation accordingly.
Would you like me to open an issue to track fixing getDiscountAmount's conditions filter in monetaryComponent.ts?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/base/monetaryComponent/monetaryComponent.test.ts` around lines 145
- 160, Update getDiscountAmount so its conditions filter treats both undefined
conditions and an explicitly empty array as applicable, using the existing
conditions-based filtering logic. Then revise the test case in
monetaryComponent.test.ts to expect the discount to be applied for a discount
without a conditions field, and remove the wording that pins the current
behavior as intentional.
| it("rounds every line item to 2 decimals and multiplies by quantity", () => { | ||
| const components: MonetaryComponent[] = [ | ||
| { monetary_component_type: MonetaryComponentType.base, amount: "33.33" }, | ||
| ]; | ||
| const breakdown = getPriceBreakdown(components, 3); | ||
| expect(breakdown).toEqual({ | ||
| basePrice: "99.99", | ||
| surcharges: "0.00", | ||
| discounts: "0.00", | ||
| subtotal: "99.99", | ||
| tax: "0.00", | ||
| total: "99.99", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rounding test doesn't actually exercise rounding.
33.33 * 3 = 99.99 requires no rounding at all, so this case doesn't verify the "rounds every line item to 2 decimals" behavior the it description claims. Use a value that needs rounding (e.g. an amount like "33.335" or "33.334") so a genuine rounding boundary is asserted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/base/monetaryComponent/monetaryComponent.test.ts` around lines 238
- 251, Update the test case around getPriceBreakdown so its component amount
requires rounding to two decimal places, such as 33.335 or 33.334, and adjust
the expected breakdown values to assert the resulting rounded amount multiplied
by the quantity. Keep the existing quantity and breakdown assertions otherwise
unchanged.
| const text = await res.text(); | ||
| try { | ||
| return await res.json(); | ||
| return JSON.parse(text) as TData; | ||
| } catch { | ||
| return (await res.text()) as TData; | ||
| return text as TData; |
| it("returns null for an explicitly empty body", async () => { | ||
| const res = new Response(null, { | ||
| headers: { "content-length": "0", "content-type": "application/json" }, | ||
| }); | ||
| await expect(getResponseBody(res)).resolves.toBeNull(); | ||
| }); |
Co-authored-by: nihal467 <57055998+nihal467@users.noreply.github.com>
Fixed in |
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
@claude[agent] convert autoRegisterComponent.test.ts to use vitest and resolve package.json. The merge conflict in package file is caused by this since this it uses node:test. |
…test Co-authored-by: Jacobjeevan <40040905+Jacobjeevan@users.noreply.github.com>
Done in
Resolved in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Utils/request/utils.ts:109
- For JSON responses with an empty body but without a readable
content-length: 0header (common in browsers due to CORS),res.text()yields an empty string and this currently falls into the JSON.parse() catch path, returning "" instead ofnull. That can leak an unexpected string intocallApi()for endpoints that legitimately return no content (e.g.TRes: void). Consider treating an empty string body asnullbefore attempting JSON.parse().
const text = await res.text();
try {
return JSON.parse(text) as TData;
} catch {
return text as TData;
src/Utils/request/utils.test.ts:27
- The empty-body test only covers the
content-length: 0case. In browserscontent-lengthis often missing/unreadable (CORS), so it would be good to also cover an empty JSON body with nocontent-lengthheader to prevent regressions once getResponseBody treats "" as empty.
it("returns null for an explicitly empty body", async () => {
const res = new Response(null, {
headers: { "content-length": "0", "content-type": "application/json" },
});
await expect(getResponseBody(res)).resolves.toBeNull();
});
Closes #16507
What
Bootstraps a fast, backend-free verification baseline:
vitest.config.mts— deliberately not reusingvite.config.mts(module-federation/PWA plugins don't apply to unit tests). Node environment by default; test files opt into jsdom per-file with// @vitest-environment jsdom(jsdom was already installed).test:unit,test:unit:watch, andtypecheck. The existingtest(snyk) script is untouched.src/Utils/decimal.test.tscharacterizes the decimal.js money helpers and proves alias +@careConfigresolution under Vitest.linter.ymlgainscache: "npm"(the only workflow missing it) plus typecheck and unit-test steps — so PRs are now gated on both.A discovery worth knowing
npx tsc --noEmitfails on a clean checkout:src/supportedBrowsers.tsis gitignored and only generated bynpm run supported-browsers(normally viadev/build). It passes locally only because a prior build generated it. Thetypecheckscript therefore chains the generation step, which makes it correct on clean checkouts and CI runners with no workflow-specific setup:Verification
npm run typecheck→ exit 0 on a freshnpm ciworktreenpm run test:unit→ 5/5 passingnpx eslint src/Utils/decimal.test.ts→ cleanFollow-up PRs (#16508, #16509, #16511, #16512) are stacked on this branch so their unit tests have the harness.
🤖 Generated with Claude Code
Summary by CodeRabbit