Skip to content

Fix compaction test error handler raising TypeError and masking the real failure - #2435

Open
Anai-Guo wants to merge 1 commit into
HolmesGPT:masterfrom
Anai-Guo:fix-compaction-handle-test-error-arity
Open

Fix compaction test error handler raising TypeError and masking the real failure#2435
Anai-Guo wants to merge 1 commit into
HolmesGPT:masterfrom
Anai-Guo:fix-compaction-handle-test-error-arity

Conversation

@Anai-Guo

@Anai-Guo Anai-Guo commented Aug 31, 2026

Copy link
Copy Markdown

handle_test_error accepts six parameters and has no **kwargs
(tests/llm/utils/property_manager.py:299):

def handle_test_error(
    request,
    error: Exception,
    eval_span=None,
    test_case=None,
    model: Optional[str] = None,
    result=None,
) -> None:

but tests/llm/test_compaction.py passes a seventh, mock_generation_config=None,
so the call raises TypeError: handle_test_error() got an unexpected keyword argument 'mock_generation_config'.

Why this matters more than a normal arity slip

The call is the body of the test's error handler:

except Exception as e:
    error = e
    handle_test_error(
        request=request,
        error=e,
        ...
        result=None,
        mock_generation_config=None,
    )
    raise

So whenever a compaction test genuinely fails, the real failure is discarded and
replaced by a TypeError originating in the error handler — the raise at the
end is never reached, and the actual assertion error never surfaces. The
centralized reporting this helper exists to provide (Braintrust logging,
property recording) is also skipped entirely for this file.

The other call sites are already correct

Call site Extra arg Status
tests/llm/test_ask_holmes.py:140
tests/llm/test_holmes_checks.py:244
tests/llm/test_compaction.py:252 mock_generation_config

test_ask_holmes.py:140 passes exactly the same keyword set as this call site
minus mock_generation_config, which makes the intended shape unambiguous.

Binding the real upstream signature against all three, with the two working
call sites as the control group:

handle_test_error (request, error, eval_span=None, test_case=None, model=None, result=None)
  TypeError tests/llm/test_compaction.py:252 (+mock_generation_config) -> got an unexpected keyword argument 'mock_generation_config'
  OK        CONTROL tests/llm/test_ask_holmes.py:140
  OK        CONTROL tests/llm/test_holmes_checks.py:244

mock_generation_config is not referenced anywhere else in the repository, so
this drops the single stale line rather than adding a parameter nothing reads.
ruff check passes, and the change is format-neutral.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Updated compaction test error handling to align with the current interface.
    • No end-user-facing behavior changes.

…to handle_test_error

handle_test_error has no mock_generation_config parameter, so the error handler
in test_compaction.py raises TypeError itself. Because the call sits inside
`except Exception as e:`, the original test failure is replaced by a TypeError
from the handler and never reported.

The two other call sites already pass only supported arguments.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@netlify

netlify Bot commented Aug 31, 2026

Copy link
Copy Markdown

Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit 89b64d5
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/6a95aa2e6d711200077e22bb
😎 Deploy Preview https://deploy-preview-2435--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Aug 31, 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a666fc0d-ef7b-4a69-9a44-d6ff1640903e

📥 Commits

Reviewing files that changed from the base of the PR and between f4ec164 and 89b64d5.

📒 Files selected for processing (1)
  • tests/llm/test_compaction.py
💤 Files with no reviewable changes (1)
  • tests/llm/test_compaction.py

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


Walkthrough

The compaction test removes the mock_generation_config=None argument from its exception handler call to handle_test_error.

Changes

Compaction Test

Layer / File(s) Summary
Update compaction test error reporting
tests/llm/test_compaction.py
The exception path no longer passes the removed mock_generation_config=None argument to handle_test_error.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 89b64

This PR removes a stale keyword argument from a test-only error-handler call; no actionable merge-blocking risk remains after normal checks and review.

🚥 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 and concisely describes the main change: fixing a TypeError in the compaction test error handler that masked the original failure.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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.

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