Skip to content

test(signature): hierarchy, receiver and default guards for change_signature (#1533, 9/9) - #2167

Merged
vitali87 merged 1 commit into
mainfrom
pr-split/split-1912-change-signature/pr-9
Oct 31, 2026
Merged

vitali87 merged 1 commit into
mainfrom
pr-split/split-1912-change-signature/pr-9

Conversation

@vitali87

@vitali87 vitali87 commented Sep 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Adds codebase_rag/tests/test_change_signature_hierarchy.py: override hierarchies, receivers and static methods, defaults that read a renamed parameter, and the guard against dropping a parameter the body still reads.
  • Tests only: no production code changes. This is the top of the stack.

Demo

No user-visible change: adds the hierarchy, receiver and default-guard tests for change_signature.

Type of Change

  • Bug fix
  • New feature
  • Performance improvement
  • Refactoring (no functional changes)
  • Documentation
  • CI/CD or tooling
  • Dependencies

None of these applies: the change adds tests only.

Related Issues

Part of #1533 (split from #1912; stack: #2163 → #2161 → #2162 → #2166 → #2164 → #2169 → #2168 → #2165 → #2167).

Test Plan

  • Unit tests pass (make test-parallel or uv run pytest -n auto -m "not integration")

  • New tests added

  • Integration tests pass (make test-integration, requires Docker)

  • Manual testing (describe below)

  • uv run python -m pytest codebase_rag/tests/test_change_signature_hierarchy.py -p no:randomly -q at 27f7ed7b9: 15 passed.

  • The full suite was not run locally. The CI Unit Tests run at 27f7ed7b9 was cancelled before it finished.

Checklist

  • PR title follows Conventional Commits format
  • All pre-commit checks pass (make pre-commit)
  • No hardcoded strings in non-config/non-constants files
  • No # type: ignore, cast(), Any, or object type hints
  • New comments and docstrings explain why, not what (see Comment Policy)

SKIP=no-commit-to-branch pre-commit run --from-ref 72e529b3e --to-ref 27f7ed7b9 (ruff, ruff-format, ty, bandit, generate-readme and the file hygiene hooks): passed. The unit-suite step of make pre-commit was not run locally.

Unchecked items:

  • The tests write their fixture source and expected text inline rather than through constants.
  • Comments were not re-audited against the policy in this pass.

Files changed

  • codebase_rag/tests/test_change_signature_hierarchy.py

Diff stats

+471 additions, -0 deletions (471 LOC)

Dependencies

This PR depends on: pr-8

Dependency graph

Merge in this order:

pr-1: feat(signature): contract expectation, shared call-site helpers and constants for change_signature (#1533, 1/9)
└── pr-2: feat(signature): parameter specs, mapping sources and the change_signature contract check (#1533, 2/9)
    └── pr-3: feat(signature): read the header, bind call arguments, render the rewrite (#1533, 3/9)
        └── pr-4: feat(signature): the change_signature operation (#1533, 4/9)
            └── pr-5: feat(signature): expose change_signature through the CLI and MCP, with docs (#1533, 5/9)
                └── pr-6: test(signature): acceptance, transaction and MCP tests for change_signature (#1533, 6/9)
                    └── pr-7: test(signature): mapping tests for change_signature (#1533, 7/9)
                        └── pr-8: test(signature): definition and renamed-parameter tests for change_signature (#1533, 8/9)
                            └── pr-9: test(signature): hierarchy, receiver and default guards for change_signature (#1533, 9/9)  <-- this PR

Summary by CodeRabbit

  • Tests
    • Expanded coverage for changing function and method signatures, including inherited methods, parameter defaults, and call sites.
    • Added checks that potentially unsafe changes are refused without altering source, while valid changes are applied consistently.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
Commits

Reviewing files that changed from the base of the PR and between fa6b6ef and 0c0bec9.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: vitali87/code-graph-rag/.coderabbit.yaml

Review profile: QUIET

Plan: Advanced

Run ID: 1575f1d4-2a00-41f5-9f3f-26f6a9f5e4cc

📥 Commits

Reviewing files that changed from the base of the PR and between 7ad8ae2 and 021bd5a.

📒 Files selected for processing (1)
  • codebase_rag/tests/test_change_signature_hierarchy.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.

Walkthrough

This change adds regression tests for change_signature. The tests cover method hierarchies, receiver handling, parameter renames and drops, annotation spelling, and call-site mapping.

Changes: Change signature regression tests

Layer / File(s) Summary
Hierarchy and call-site mapping
codebase_rag/tests/test_change_signature_hierarchy.py
Tests hierarchy rewrites, mismatched overrides, receiver and static-method handling, and generator call sites reported as unmapped.
Parameter renaming and annotations
codebase_rag/tests/test_change_signature_hierarchy.py
Tests refusal when a default reads a renamed parameter, valid renaming when it does not, and annotation handling across overrides.
Dropped parameter handling
codebase_rag/tests/test_change_signature_hierarchy.py
Tests refusal when the body reads a dropped parameter and successful removal when it is unused, including matching attribute names and keyword labels.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Merge Risk: Minimal · previous review 021bd

This change adds test coverage for change_signature and does not alter runtime behavior. No merge-blocking risk was identified.

Pre-merge checks: Passed 4
Check name Status Explanation
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.
Title check Passed The title clearly identifies the test scope: hierarchy, receiver, and default guards for change_signature. The issue and stack position add context without making the title misleading.
Description check Passed The description includes the required sections, explains that the change adds tests only, records the focused test and pre-commit results, and notes that the full suite was not run. The unchecked chec…
Full details: Description check

Explanation

The description includes the required sections, explains that the change adds tests only, records the focused test and pre-commit results, and notes that the full suite was not run. The unchecked checklist items are explicitly documented.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@vitali87
vitali87 added this pull request to stack #2170 September 22, 2026 23:23
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch from 97e47fe to 4c11178 Compare October 4, 2026 15:32

vitali87 commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner Author

Unit Tests (windows-latest, py3.13) fails on test_stack_quiet_wait.py::test_the_c_library_message_is_kept_at_debug_level[close]. This PR does not change that code. The same test fails on main's CI at ff3e0b7, which was only a version bump. It passes on Windows py3.12 and on every other platform. The test's reset case is already skipped on Windows because mgclient does not always write to stderr there. No fix exists yet. I'll re-run the job once when the run finishes.

Copy link
Copy Markdown
Owner Author

PR Title Convention failed on 880ffea without checking the title. Its first API call got API rate limit exceeded for installation from GitHub, so the action never evaluated anything. The title itself (test(signature): ...) uses an allowed type and matches the subject pattern. Nothing in this PR can fix a rate limit; I'll re-run the job once when the run finishes.


Generated by Claude Code

@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch from 880ffea to bfb067e Compare October 18, 2026 18:36
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch from bfb067e to d7e1414 Compare October 18, 2026 20:35
@vitali87 vitali87 removed the merge_conflict label Oct 18, 2026 — with Claude
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch 2 times, most recently from 6eae567 to f904ece Compare October 20, 2026 19:25
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch from f904ece to 5856c73 Compare October 21, 2026 04:02
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch 2 times, most recently from 3b31d9d to fa6b6ef Compare October 28, 2026 21:16
Base automatically changed from pr-split/split-1912-change-signature/pr-8 to main October 30, 2026 14:00
@vitali87
vitali87 force-pushed the pr-split/split-1912-change-signature/pr-9 branch from fa6b6ef to 0c0bec9 Compare October 30, 2026 14:00

Copy link
Copy Markdown
Owner Author

SonarCloud Analysis failed on 0c0bec97f, but not because of this PR's code. In that job the full suite passed (24575 passed, 75 skipped). The scanner then stopped before it analysed anything, because SonarCloud could not return the PR record: ERROR Something went wrong while trying to get the pullrequest with key '2167': An unexpected error occurred. Please try again later. This PR adds one test file and does not touch the Sonar setup, so there is nothing to fix here. It may be because the PR was retargeted from pr-8 to main a few minutes before the scan; the other SonarCloud runs from the same hour, on main and on other PRs, succeeded or are still running. I've re-run the failed job once.


Generated by Claude Code

@sonarqubecloud

Copy link
Copy Markdown

@vitali87

Copy link
Copy Markdown
Owner Author

@greptile review

@greptile-apps

This comment has been minimized.

@greptile-apps

greptile-apps Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; no blocking issues were found.

Summary

Adds tests for change_signature; production code is unchanged.

  • Signature-change tests cover inherited methods and guarded edits.

No actionable issues found. Tests were not run during this review.

Reviews (1) · Last reviewed commit: "test(signature): hierarchy, receiver and..." · Reviewed by Greptile

@vitali87
vitali87 merged commit f6372a1 into main Oct 31, 2026
65 of 68 checks passed
@vitali87
vitali87 deleted the pr-split/split-1912-change-signature/pr-9 branch October 31, 2026 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant