test: scoring.ts, rule-engine.ts 핵심 모듈 유닛 테스트 추가

[let-sunny]

Summary

• scoring.ts에 23개 유닛 테스트 신규 추가 (기존 0개)
• rule-engine.test.ts에 15개 테스트 추가 (기존 1개 → 16개)
• 전체 테스트 수: 388개 → 426개 (+38)

scoring.test.ts (신규, 23 tests)

• calculateScores: zero issues→100%, severity 카운팅, density 가중치, diversity 패널티, combined 공식 검증, floor clamping, nodeCount 엣지 케이스 (0, 1)
• Grade 경계: S(100%), F(<50%), gradeToClassName 변환
• formatScoreSummary, getCategoryLabel, getSeverityLabel
• buildResultJson 필드 구조 + issuesByRule 집계 검증

rule-engine.test.ts (확장, +15 tests)

• targetNodeId: 서브트리 스코핑, dash→colon 정규화, 존재하지 않는 노드 에러
• excludeNodeTypes / excludeNodeNames 필터링
• enabledRules / disabledRules / config.enabled 필터링
• depthWeight 보간 (root vs leaf 점수 차이)
• 트리 순회: nodeCount, maxDepth, 빈 document
• 에러 복원력: 룰이 throw해도 분석 계속 진행
• analyzeFile convenience function + custom configs

Test plan

• pnpm test:run — 426 tests passed
• pnpm lint — 타입 체크 통과
• 기존 테스트 깨짐 없음

Closes #55

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Expanded rule-analysis tests: state isolation across runs, subtree targeting, node exclusion, traversal counts (nodeCount/maxDepth), empty-document case, error resilience when rules throw, rule enable/disable behaviors, and convenience file-analysis coverage.
  • Added comprehensive scoring tests: zero-issue and nodeCount edge cases, severity/category counts, density/diversity weighting and clamping, grade/label formatting, presentation helpers, and JSON output validations.

[coderabbitai]

Warning

Rate limit exceeded

@let-sunny has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 37 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0e9cbf0d-233a-4051-9bd7-c13fe061d61e

📥 Commits

Reviewing files that changed from the base of the PR and between f7a3c4e and 12407d0.

📒 Files selected for processing (1)

• src/core/engine/rule-engine.test.ts

📝 Walkthrough

Walkthrough

Adds comprehensive unit tests for the core analysis engine and scoring logic: new suites validate RuleEngine.analyze/analyzeFile behaviors (filtering, subtree selection, error resilience) and scoring utilities (density/diversity/severity weighting, grading, and presentation helpers).

Changes

Cohort / File(s)	Summary
RuleEngine tests src/core/engine/rule-engine.test.ts	New ~350-line test suite covering RuleEngine.analyze and analyzeFile: per-analysis state isolation, targetNodeId subtree restriction (dash→colon normalization and missing-target error), excludeNodeTypes/excludeNodeNames, enabledRules/disabledRules, traversal metrics (nodeCount, maxDepth, empty-doc case), handling when a rule check throws (logs to console.error), and custom configs disabling all rules.
Scoring tests src/core/engine/scoring.test.ts	New ~400-line test suite for calculateScores and helpers: zero-issue behavior (grade S, categories 100%), severity-weighted density, diversity vs. concentration differences, combined formula verification (density * 0.7 + diversity * 0.3 with clamping), category floor enforcement (min 5 when issues exist), nodeCount edge cases (0/1), and presentation/helper tests (gradeToClassName, formatScoreSummary, getCategoryLabel, getSeverityLabel, buildResultJson and issuesByRule aggregation).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• refactor: replace module-level mutable state with per-analysis RuleContext state #56: New RuleEngine.analyze/analyzeFile tests exercise per-analysis state isolation and target-subtree behavior introduced by that PR.
• refactor: unify missing-component with 4-stage checks (#17) #22: Tests relate to RuleEngine analyze behavior and interactions with resetMissingComponentState() modified in that PR.

Poem

🐰 I hopped through tests both brave and bright,
I scoped each rule and counted every bite,
Density, diversity, scores in tune,
I chased a bug beneath the moon,
Now code sleeps safe till morning light 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title references test additions for scoring.ts and rule-engine.ts core modules, which matches the primary changes of adding 23 unit tests to scoring.test.ts and 15 tests to rule-engine.test.ts.
Linked Issues check	✅ Passed	PR addresses all coding requirements from issue #55: comprehensive unit tests for calculateScores (weights, floor handling, grade boundaries, edge cases), analyze (node filtering, subtree scoping, rule filtering, error resilience), and analyzeFile.
Out of Scope Changes check	✅ Passed	All changes are within scope: two new test files and one test file update focused on core module unit testing with no unrelated code modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/core-module-unit-tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 258-286: The test's main assertion is guarded by an if and can
pass vacuously if issues aren't found; update the test around analyzeFile/result
to assert presence of the expected issues before comparing scores by adding
explicit assertions like expect(issueAtChild).toBeDefined() and
expect(issueAtGC).toBeDefined() (or remove the conditional and let the expect
calls fail) so the comparison of issueAtChild.calculatedScore and
issueAtGC.calculatedScore runs only when both issues are present; locate the
variables issueAtChild, issueAtGC and the analyzeFile call to apply this change.
- Around line 359-382: The test currently doesn't inject an error — add a rule
that throws inside its check() and ensure analyzeFile(file) still completes:
create a mock rule (e.g., { id: "thrower", check: () => { throw new
Error("boom") }, applicable: () => true }) and register or inject it into the
RuleEngine/rules collection used by analyzeFile (or spy/mock an existing rule's
check method to throw), then call analyzeFile(file), assert result.issues and
result.nodeCount as before, and keep the console.error spy to verify the thrown
error was logged without halting analysis.

In `@src/core/engine/scoring.test.ts`:
- Around line 270-288: The helper function scoreWithPercentage is dead code
(defined but never used); remove the unused function scoreWithPercentage from
src/core/engine/scoring.test.ts, or alternatively implement the intended
parameterized tests that call scoreWithPercentage with target percentages and
assert expected grades (use existing helpers makeResult and calculateScores to
produce results and verify grade boundaries). Ensure any added tests reference
scoreWithPercentage, makeResult, and calculateScores so the helper is actually
exercised; otherwise delete the helper to keep the test file clean.
- Around line 295-319: Replace the non-null assertion on rulesPerCat[cat] in the
nested loop with an explicit safe lookup: retrieve const rules =
rulesPerCat[cat] and skip/continue if rules is undefined before iterating, then
iterate over rules instead; this removes the `!` assertion while keeping the
same behavior and references the existing symbols rulesPerCat, categories, and
the nested for (const ruleId ...) loop in the test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d8aa6f2e-bfbc-4729-a53d-421a69b72c0a

📥 Commits

Reviewing files that changed from the base of the PR and between 089ad4a and 542910a.

📒 Files selected for processing (2)

• src/core/engine/rule-engine.test.ts
• src/core/engine/scoring.test.ts

[coderabbitai]

♻️ Duplicate comments (1)

src/core/engine/rule-engine.test.ts (1)

160-164: ⚠️ Potential issue | 🟡 Minor

Strengthen filtering tests to avoid vacuous passes.

Both tests only assert filtered output when baseline issues exist, so they can pass without validating the filter behavior. Fail fast on baseline presence, then assert exclusion.

Suggested test hardening

@@
-    expect(textIssuesExcluded.length).toBeLessThanOrEqual(textIssuesAll.length);
-    // If TEXT generated any issues, they should be filtered out
-    if (textIssuesAll.length > 0) {
-      expect(textIssuesExcluded.length).toBe(0);
-    }
+    expect(textIssuesAll.length).toBeGreaterThan(0);
+    expect(textIssuesExcluded.length).toBe(0);
@@
-    // default-name issues should be absent when disabled
-    if (defaultNameAll.length > 0) {
-      expect(defaultNameDisabled.length).toBe(0);
-    }
+    expect(defaultNameAll.length).toBeGreaterThan(0);
+    expect(defaultNameDisabled.length).toBe(0);

Also applies to: 232-234

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/engine/rule-engine.test.ts` around lines 160 - 164, The tests
currently only assert that textIssuesExcluded.length <= textIssuesAll.length and
conditionally check exclusion, which allows vacuous passes; change the
assertions to first fail fast if the baseline has no issues (e.g., require
textIssuesAll.length > 0 using expect(textIssuesAll.length).toBeGreaterThan(0)),
then assert the exclusion behavior (expect(textIssuesExcluded.length).toBe(0));
apply the same pattern to the duplicate block that uses the other pair of
variables at 232-234 so the filter behavior is actually validated when a
baseline exists.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 160-164: The tests currently only assert that
textIssuesExcluded.length <= textIssuesAll.length and conditionally check
exclusion, which allows vacuous passes; change the assertions to first fail fast
if the baseline has no issues (e.g., require textIssuesAll.length > 0 using
expect(textIssuesAll.length).toBeGreaterThan(0)), then assert the exclusion
behavior (expect(textIssuesExcluded.length).toBe(0)); apply the same pattern to
the duplicate block that uses the other pair of variables at 232-234 so the
filter behavior is actually validated when a baseline exists.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2df8a647-f0c6-47d4-9021-963cc99dead8

📥 Commits

Reviewing files that changed from the base of the PR and between 542910a and e4fb283.

📒 Files selected for processing (2)

• src/core/engine/rule-engine.test.ts
• src/core/engine/scoring.test.ts

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/core/engine/rule-engine.test.ts (2)

165-182: ⚠️ Potential issue | 🟠 Major

excludeNodeNames test does not prove exclusion behavior.

This only asserts 0 after filtering; it never proves the ignored node produced issues before filtering. Add a baseline run and assert those issues exist first.

Proposed fix

   it("skips nodes matching excludeNodeNames pattern", () => {
@@
     const file = makeFile({ document: doc });
-    const result = analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] });
+    const baseline = analyzeFile(file);
+    const result = analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] });
+
+    const ignoredIssuesBaseline = baseline.issues.filter(
+      (i) => i.violation.nodeId === "i:1"
+    );
 
     const ignoredIssues = result.issues.filter(
       (i) => i.violation.nodeId === "i:1"
     );
+    expect(ignoredIssuesBaseline.length).toBeGreaterThan(0);
     expect(ignoredIssues.length).toBe(0);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/engine/rule-engine.test.ts` around lines 165 - 182, The test "skips
nodes matching excludeNodeNames pattern" doesn't prove exclusion because it
never verifies the ignored node produced issues before applying
excludeNodeNames; update the test to first run analyzeFile(file) without the
exclude option and assert there are one or more issues for node id "i:1" (using
makeNode/makeFile and result.issues filtering), then run analyzeFile(file, {
excludeNodeNames: ["IgnoreMe"] }) and assert the filtered ignoredIssues length
is 0 to demonstrate the exclusion behavior.

---

188-206: ⚠️ Potential issue | 🟠 Major

enabledRules test can pass vacuously with zero issues.

If analysis returns no issues, the loop never asserts anything. Add a non-empty guard so the test actually validates rule filtering.

Proposed fix

   it("runs only enabledRules when specified", () => {
@@
     const result = analyzeFile(file, { enabledRules: ["default-name"] });
 
+    // Ensure this test cannot pass vacuously
+    expect(result.issues.length).toBeGreaterThan(0);
+
     // All issues should be from default-name only
     for (const issue of result.issues) {
       expect(issue.violation.ruleId).toBe("default-name");
     }
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/engine/rule-engine.test.ts` around lines 188 - 206, The test "runs
only enabledRules when specified" can pass vacuously when result.issues is
empty; update the assertion to first assert there is at least one issue (e.g.,
expect(result.issues.length).toBeGreaterThan(0) or similar) before iterating,
then keep the existing loop that ensures every issue.violation.ruleId ===
"default-name" using analyzeFile, enabledRules and result.issues to locate the
code to change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 234-251: Update the test "skips rules disabled in config (enabled:
false)" to include a positive control: first run analyzeFile with the same
fixture but with the "no-dev-status" rule explicitly enabled (e.g., by passing a
config override or setting the analyzer options) and assert that result.issues
contains at least one issue whose violation.ruleId === "no-dev-status"; then run
the existing default analyzeFile call to assert zero issues. Reference the
existing test name and the analyzeFile function and the rule id "no-dev-status"
when making the change.

---

Duplicate comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 165-182: The test "skips nodes matching excludeNodeNames pattern"
doesn't prove exclusion because it never verifies the ignored node produced
issues before applying excludeNodeNames; update the test to first run
analyzeFile(file) without the exclude option and assert there are one or more
issues for node id "i:1" (using makeNode/makeFile and result.issues filtering),
then run analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] }) and assert the
filtered ignoredIssues length is 0 to demonstrate the exclusion behavior.
- Around line 188-206: The test "runs only enabledRules when specified" can pass
vacuously when result.issues is empty; update the assertion to first assert
there is at least one issue (e.g.,
expect(result.issues.length).toBeGreaterThan(0) or similar) before iterating,
then keep the existing loop that ensures every issue.violation.ruleId ===
"default-name" using analyzeFile, enabledRules and result.issues to locate the
code to change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4ea6ecaf-4ff2-431d-b731-cd6e963010d4

📥 Commits

Reviewing files that changed from the base of the PR and between e4fb283 and f7a3c4e.

📒 Files selected for processing (1)

• src/core/engine/rule-engine.test.ts