feat: add plan to fill test-coverage gaps#365
Open
phillipc wants to merge 9 commits into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR establishes comprehensive code coverage infrastructure for the TKO monorepo by configuring Vitest V8 coverage reporting, adding 70+ new test cases across core libraries to measure baseline behavior, regenerating the coverage report, and documenting a phased gap-fill plan with measurable targets. ChangesCoverage Infrastructure and Test Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
parseJson lacked a spec entirely (string.ts: 0% lines / 0% branches). Adds stringBehaviors.ts covering: valid JSON parsing, primitives and arrays, leading/trailing whitespace, empty and whitespace-only strings, non-string runtime input (returns null instead of coercing or throwing), and malformed JSON (rethrows JSON.parse — pinned so a future 'be helpful and return null' change is caught). Lifts utils/src/string.ts from 0% to 100% lines and branches. Adversarial-review audit: subagent reviewed; cleared as solid (assertions exercise production code, type casts intentional).
Subscription.ts had 84.61% lines / 50% functions: the TC39
Observable-style unsubscribe() alias and the closed getter were never
exercised by any spec.
Adds two it() blocks to subscribableBehaviors:
- unsubscribe() acts as an alias for dispose(): closed flips true and
later notifications do not reach the callback.
- closed reports false before dispose, true after.
In combination with existing computed/observable specs that exercise
disposeWhenNodeIsRemoved(), this lifts Subscription.ts to 100% lines
and branches.
Adversarial-review audit: an earlier draft asserted dispose() was
idempotent ("survive a second dispose"). Subscription.dispose() has no
_isDisposed guard, so a second call re-fires _disposeCallback and could
emit duplicate 'asleep' notifications via afterSubscriptionRemove. The
weak "does not throw" assertion was removed to avoid pinning unverified
behavior; if double-dispose hardening is wanted it lands as a separate
production fix with its own spec.
submit.ts had 81.81% lines / 50% branches: the throw-on-non-function guard and the 'handler returned true → skip preventDefault' branch were uncovered. Adds three it() blocks: - Bound value that is not a function throws with the documented message. - Handler returning true leaves event.defaultPrevented false (test attaches its own listener to read defaultPrevented BEFORE calling its own preventDefault, so the assertion observes the binding's behavior, not the test's own cleanup). - Handler returning a non-true value sets event.defaultPrevented true. Lifts submit.ts from 81.81% to 90.90% lines and from 50% to 83.33% branches. Remaining uncovered branches are the IE-only event.returnValue fallback (no preventDefault), unreachable in headless chromium. Adversarial-review audit: subagent verified the preventDefault tests read defaultPrevented before the test's own preventDefault call and so genuinely observe binding behavior. Subagent flagged a pre-existing state leak in this file (no afterEach restoring options.bindingProvider- Instance); that is an established pattern across binding.* specs and is out of scope for this commit.
BindingHandlerObject.ts had 77.77% lines / 66.66% branches: the two
options.onError calls (object + extraneous value, and non-string non-
object key) were never exercised, and the dotted-name lookup in get()
was lightly covered.
Adds BindingHandlerObjectBehaviors with six it() blocks:
- set(name, value) registers a single handler (lookup via get).
- set(object) registers multiple handlers via Object.assign.
- set(object, value) reports onError with the documented message.
- set(undefined) and set(42) report onError with the documented message.
- get('attr.title') and get('attr.style.color') resolve to the root
segment 'attr' (pins the dotted-handler convention).
- get('nope') returns undefined for unknown handlers.
beforeEach captures the original options.onError; afterEach restores it
so a stub does not leak into other specs even if a test fails mid-run.
Lifts BindingHandlerObject.ts from 77.77% to 100% lines and from 66.66%
to 91.66% branches. Remaining uncovered branch is the Object.assign
fall-through after the (object + extra value) error path, which the
production code intentionally still runs.
Adversarial-review audit: subagent verified options.onError stub setup
and teardown isolate per test; no leakage path identified.
Phase 1 of plans/coverage-gap-fill.md is complete (4 of 5 items):
- utils/string.ts: 0.00% → 100.00% lines
- observable/Subscription.ts: 84.61% → 100.00% lines
- provider/BindingHandlerObject.ts: 77.77% → 100.00% lines
- binding.core/submit.ts: 81.81% → 90.90% lines
Overall: 92.71% → 92.96% statements, 87.63% → 87.95% branches,
90.29% → 90.72% functions, 92.82% → 93.10% lines.
Phase 1 item 2 (binding.core/descendantsComplete.ts) is dropped: the
plan was annotated 'Allegedly dead code' and verification confirms it.
applyBindings.ts:405-422 special-cases the descendantsComplete binding
name and invokes the value accessor directly, bypassing the
DescendantsCompleteHandler subclass entirely. onDescendantsComplete()
has no caller in the framework. Writing a unit test that constructs the
class and calls the method would lift the line counter without proving
anything user-facing — that violates the plan's adversarial checklist
('coverage gamed'). Removal of the dead code, or rewiring the binding
to use the lifecycle method, belongs in its own PR.
Adversarial-review audit: not in scope (docs + plan only).
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
1 participant
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.
This pull request adds a comprehensive plan in
plans/coverage-gap-fill.mdto systematically close test coverage gaps across the public@tko/*packages. The plan prioritizes files and packages with the lowest coverage, outlines measurable goals, and details a phased approach for adding targeted tests. It also specifies verification steps and adversarial review guidelines to ensure new tests are meaningful and robust.Coverage gap analysis and prioritization:
Phased approach for test additions:
Verification and review discipline:
Summary by CodeRabbit
Tests
Chores