test(node-4020): config-agnostic assertions + generic org-creation seam isolation - #4021
Conversation
…am isolation Applies the epic's 4 homogeneity rules to the test files a consumer currently has to adapt: - billing quota: force an explicit dev-grade NODE_ENV (set/restore) instead of riding whatever ambient value a consumer's own test runner uses. - billing subscription schema: derive plan ids from the loaded config instead of a hardcoded devkit default; add a synthetic-catalogue block proving the schema is config-driven, not a re-labeled default. - organizations email-verification + silent-catch: mock the org-creation event emitter generically (the hook mechanism itself) instead of leaving it real, so no consumer-named listener is ever needed to isolate these tests; add coverage proving a generically-registered listener receives the documented payload. - invitations open-signup: the one assertion riding the ambient config.invitations.userFacing default now installs its own value (set/restore) — the true/false split is already covered elsewhere in the file. - new organizationAbilities unit suite: admin/user × owner/admin/member/ no-membership matrix over the CASL ability builder, generic policy coverage the stack lacked. Closes #4020
Simplify pass over the Node#4020 test-homogeneity batch: - organizations.emailVerification: drop the new real-EventEmitter seam block and switch to the same bare-double lib/events.js mock already used elsewhere in the module — organizations.service.signup.unit.tests.js already covers the organization.created/organization.provisioned payload shape and ordering generically, so this file only needed isolation. - organizations.abilities: extract a callAbilities(user, membership) helper to remove the repeated can/cannot spy boilerplate across the matrix. - invitations.integration: fold the userFacing save/restore into the describe block's existing beforeEach/afterEach (same pattern already used for sign.up/sign.cap), dropping the test's bespoke try/finally. - billing.quota: hoist the NODE_ENV save/restore to beforeAll/afterAll — the value never changes per test. Re-verified: full test:coverage suite green, plus the config/NODE_ENV- independent files re-checked under a synthetic non-dev NODE_ENV.
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR makes billing and invitation tests configuration-independent, isolates organization event seams, and adds comprehensive ChangesTest hermeticity and policy coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Verification detailFull suite (bare devkit-default config) — `npm run test:coverage`: Synthetic-consumer profile — the 4 files whose fix depends on config/NODE_ENV independence (`billing.quota.unit.tests.js`, `billing.unit.tests.js`, `organizations.service.silent.catch.unit.tests.js`, `organizations.abilities.unit.tests.js`), re-run under a non-dev-grade `NODE_ENV` unknown to this repo's config (no override file exists for it — matching how a downstream consumer's own project name would behave) + a strong JWT secret so the app config still boots: The remaining two files' config-agnosticism (rule 2's org-creation-seam isolation, rule 1+3's `userFacing` set/restore) is proved via in-file synthetic-config blocks (a rebuilt schema against a synthetic plan catalogue; the dedicated userFacing:true/false block) — both already included in the 220/2909 full-suite run above, since they don't depend on the outer process's ambient `NODE_ENV`. Discoveries (out of scope for this PR, no fix applied)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4021 +/- ##
==========================================
+ Coverage 93.80% 94.01% +0.20%
==========================================
Files 170 170
Lines 5810 5810
Branches 1860 1859 -1
==========================================
+ Hits 5450 5462 +12
+ Misses 290 285 -5
+ Partials 70 63 -7
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…logue Pre-push panel P1: rule 1 was applied to the two SubscriptionUpdate tests only, but the 'Subscription schema' and 'Subscription schema — meter fields' blocks' shared beforeEach fixtures still hardcoded plan: 'free'. Since the schema builds its plan enum from config.billing.plans, every one of the ~15 tests riding that fixture would fail for a consumer whose catalogue lacks 'free'. Derive a fixturePlan from the loaded config instead (defaultPlan when it's actually a member of the catalogue, else plans[0]) and use it in both fixtures. 'should default plan to free' is untouched — it explicitly deletes subscription.plan before parsing, so it tests the schema's own Zod default (currently hardcoded 'free' in production code, unrelated to this fixture) rather than the fixture value. Extends the synthetic-consumer-catalogue block with a test that applies the same fixturePlan derivation to a catalogue containing no devkit-shipped plan id at all, proving the mechanism the fixtures now rely on survives it.
|
@coderabbitai full review |
|
…ue fix CodeRabbit was rate-limited on the P1 fix push, so the fallback Claude reviewer covered it (verdict: OK with nits). Two nits addressed: - Document the degenerate single-distinct-plan-catalogue case for nonDefaultPlan's `?? plans[0]` fallback — there's no code fix possible (no other plan exists to pick in that case), so this clarifies the round-trip guarantee still holds either way rather than adding branching that can't actually produce a better value. - Drop the synthetic-catalogue block's afterAll(jest.resetModules()) — dead cleanup, it's the last describe block in the file.
Update — P1 fix from pre-push adversarial panelThe panel caught a real gap: rule 1 was applied to the two `SubscriptionUpdate` tests only, but the shared `beforeEach` fixtures in the `'Subscription schema'` and `'Subscription schema — meter fields'` blocks in `billing.unit.tests.js` still hardcoded `plan: 'free'` — ~15 tests riding that fixture would fail for any consumer whose plan catalogue lacks `'free'`. Fixed by deriving a `fixturePlan` from the loaded config (`defaultPlan` when it's actually in the catalogue, else `plans[0]`) and using it in both fixtures. `'should default plan to free'` is untouched on purpose — it deletes `subscription.plan` before parsing, so it's testing the schema's own Zod `.default('free')` (a production-code fact, unrelated to the fixture) — flagged as a discovery above, not fixed here (no production code touched in this PR). Extended the synthetic-catalogue block with a test that applies the exact same `fixturePlan`-style derivation to a catalogue with zero devkit-shipped plan ids, proving the mechanism survives it. CodeRabbit was rate-limited on both the fix push and the follow-up nit push (re-prompted once, still rate-limited after another backoff wait) — per this repo's reviewer-fallback policy, a Claude-backed reviewer covered the full PR diff instead (different model family from the kimi pre-push gate). Verdict: OK with nits — both nits addressed in the follow-up commit (documented the degenerate single-plan-catalogue edge case for the `nonDefaultPlan` fallback; dropped a dead `afterAll(jest.resetModules())` cleanup). Re-verified: full `test:coverage` green (220 suites / 2910 tests), 0 review threads, CI green. |
1 participant
Summary
modules/billing/tests/billing.quota.unit.tests.js,modules/billing/tests/billing.unit.tests.js,modules/invitations/tests/invitations.integration.tests.js,modules/organizations/tests/organizations.emailVerification.unit.tests.js,modules/organizations/tests/organizations.service.silent.catch.unit.tests.js) and adds a neworganizationAbilitiesunit suite (modules/organizations/tests/organizations.abilities.unit.tests.js).Scope
nonelowValidation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Notes for reviewers
Per-rule breakdown:
billing.unit.tests.js'sSubscriptionUpdatetests now derive the plan id from the loaded config instead of a hardcoded devkit-default plan id; a new nested block rebuilds the schema against a synthetic plan catalogue to prove it's genuinely config-driven, not a re-labeled default.organizations.emailVerification.unit.tests.jsandorganizations.service.silent.catch.unit.tests.jsnow mock the org-creation event emitter (a plain hook/registry any consumer module can subscribe to) with a bare double, rather than letting the real singleton run — the same convention already used byorganizations.service.signup.unit.tests.js, which already covers the emitted-payload contract.billing.quota.unit.tests.jsnow sets/restores an explicit dev-gradeNODE_ENVfor the suite instead of depending on whichever value a consumer's own test runner happens to use — the response helper this middleware calls only serializes a debug field in a dev-grade env.invitations.integration.tests.js's one test that implicitly rode the ambientconfig.invitations.userFacingdefault now installs its own value (the true/false split is already covered elsewhere in the file).organizations.abilities.unit.tests.js— admin/user × owner/admin/member/no-membership matrix over the CASL ability builder, generic policy coverage the stack lacked.Verified both on the bare devkit-default config and against synthetic config overrides (different plan catalogue, non-dev-grade
NODE_ENV) — details in the first PR comment.Summary by CodeRabbit