Skip to content

test(edit_block): fix occurrence test false positives - #639

Open
aim9sour wants to merge 1 commit into
wonderwhy-er:mainfrom
aim9sour:fix/edit-block-occurrences-test
Open

test(edit_block): fix occurrence test false positives#639
aim9sour wants to merge 1 commit into
wonderwhy-er:mainfrom
aim9sour:fix/edit-block-occurrences-test

Conversation

@aim9sour

@aim9sour aim9sour commented Aug 19, 2026

Copy link
Copy Markdown

Summary

  • update test-edit-block-occurrences.js to assert the current plain-text edit_block preview response instead of the pre-file-preview success string
  • update the empty-search case to expect the current schema-validation rejection
  • propagate runTests() failures to the process exit code so the subprocess test runner cannot report an assertion failure as passed

Why

The test had drifted across two intentional contract changes:

  • the file-preview work changed successful plain-text edits to return a [Reading ...] preview and updated other edit tests, but this occurrence test kept asserting the old Successfully applied ... text
  • empty old_string values are intentionally treated as omitted by EditBlockArgsSchema

Because the direct entrypoint ignored the boolean returned by runTests(), those stale assertions printed failures while the process still exited 0, so test/run-all-tests.js counted the file as passed.

Verification

  • reproduced on upstream main: Test 3 raised ERR_ASSERTION while node test/test-edit-block-occurrences.js exited 0
  • after exit propagation, the remaining stale empty-search assertion correctly made the process exit 1
  • after updating the stale contracts, node test/test-edit-block-occurrences.js passes all 6 cases with exit 0
  • git diff --check passes
  • full npm test runner reports 47/47 passed, exit 0; investigation also exposed a separate pre-existing hidden failure in test-allowed-directories.js, which is intentionally not bundled into this PR

Summary by CodeRabbit

  • Tests
    • Updated edit-operation tests to validate successful results consistently.
    • Improved empty-search coverage to verify input validation errors.
    • Enhanced direct test execution with clear success and failure exit statuses.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: aff77f8d-4152-4874-8e1e-ab7d23c660d9

📥 Commits

Reviewing files that changed from the base of the PR and between 9bd8422 and e566391.

📒 Files selected for processing (1)
  • test/test-edit-block-occurrences.js

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The edit block occurrence tests now use a shared success-result assertion, expect ZodError for empty searches, and return explicit process exit codes for direct execution.

Changes

Edit block test updates

Layer / File(s) Summary
Shared edit success assertions
test/test-edit-block-occurrences.js
Added a shared assertion for file-preview success results and applied it to exact-occurrence, header-context, and footer-context tests.
Validation and execution outcomes
test/test-edit-block-occurrences.js
The empty-search test now expects handleEditBlock to reject with ZodError. Direct execution now exits with status 0 or 1 based on the test result.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to e5663

This localized test-only change corrects stale expectations and ensures assertion failures affect the test process exit code; the targeted and full test suites pass, so no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the edit_block occurrence test fix and the false-positive issue addressed by the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@aim9sour

Copy link
Copy Markdown
Author

Overlap note: #551 predates this PR and standardizes direct test entrypoints so a resolved false produces a failing process exit status. This PR has additional current-v0.2.47 fixes not present in #551: the occurrence tests still assert the pre-preview Successfully applied N edits response contract, and the empty old_string case now rejects at Zod validation. Those stale assertions are what exposed the hidden failure once exit propagation was enforced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant