Skip to content

Fix two PR #1543 tests with wrong assertion targets blocking main CI#1582

Merged
JSv4 merged 1 commit into
mainfrom
fix/pr1543-test-mock-targets
May 8, 2026
Merged

Fix two PR #1543 tests with wrong assertion targets blocking main CI#1582
JSv4 merged 1 commit into
mainfrom
fix/pr1543-test-mock-targets

Conversation

@JSv4
Copy link
Copy Markdown
Collaborator

@JSv4 JSv4 commented May 8, 2026

Summary

Two tests added in #1543 have been failing on main and blocking pytest (and therefore codecov) on every PR. Both tests assert against the wrong target:

  1. test_callable_resolves_via_from_function (test_llms_typing_coverage.py) patches opencontractserver.llms.api.ToolAPI.from_function, but _resolve_tools calls CoreTool.from_function directly. The mock never intercepts, so _resolve_tools returns real CoreTool instances and assertEqual(result, [sentinel]) fails. Patch CoreTool.from_function instead.

  2. test_add_document_note_tool_passes_none_when_corpus_absent (test_llms_typing_behavior_guards.py) does a global file scan with assertNotIn("requires the agent to be scoped to a corpus"), but that string legitimately exists for add_exact_string_annotations (a different tool that does require a corpus). Scope the assertion to the body of add_document_note_tool by slicing from its def to the next async def at the same indent level.

Test plan

  • pytest opencontractserver/tests/test_llms_typing_coverage.py::TestResolveToolsCallerForms::test_callable_resolves_via_from_function passes
  • pytest opencontractserver/tests/test_llms_typing_behavior_guards.py::TestAddDocumentNoteToolCorpusOptional::test_add_document_note_tool_passes_none_when_corpus_absent passes
  • Backend CI on this PR is green (was red on main and every downstream PR before this fix)

Two tests added in PR #1543 (Type-check opencontractserver.llms modules)
have been failing on main, blocking pytest (and therefore codecov)
on every PR. Both tests assert against the wrong target:

1. test_callable_resolves_via_from_function patches
   opencontractserver.llms.api.ToolAPI.from_function, but
   _resolve_tools calls CoreTool.from_function directly. The mock
   never intercepts the actual call site, so _resolve_tools returns
   real CoreTool instances and the assertEqual against the sentinel
   fails. Patch CoreTool.from_function instead.

2. test_add_document_note_tool_passes_none_when_corpus_absent does a
   global file scan with assertNotIn("requires the agent to be scoped
   to a corpus"), but that string legitimately exists for
   add_exact_string_annotations (a different tool that does require
   a corpus). Scope the assertion to just the body of
   add_document_note_tool by slicing the file from its def to the
   next async def at the same indent level.
@claude
Copy link
Copy Markdown

claude Bot commented May 8, 2026

Code Review

Overview

This is a targeted two-test fix that unblocks CI after #1543 introduced tests with incorrect assertion/patch targets. No production code is changed — both changes are test-only. The root cause analysis in the PR description is accurate.


Fix 1 — test_callable_resolves_via_from_function (correct)

Root cause confirmed. api.py:713 shows _resolve_tools calling CoreTool.from_function(tool) directly:

resolved.append(CoreTool.from_function(tool))

ToolAPI.from_function is a thin wrapper that ultimately calls CoreTool.from_function, but _resolve_tools bypasses ToolAPI entirely. Patching ToolAPI.from_function was therefore a no-op at the real call site.

The fix is correct. Patching opencontractserver.llms.api.CoreTool.from_function intercepts the right symbol, and from_fn.assert_called_once_with(my_tool) validates the call arguments too — good.


Fix 2 — test_add_document_note_tool_passes_none_when_corpus_absent (correct, minor fragility concern)

Root cause confirmed. pydantic_ai_agents.py:2497 contains:

"add_exact_string_annotations requires the agent to be scoped to a corpus"

A global file scan for that substring false-positives on the other tool. The fix is correct in concept.

The body-extraction logic works for the current code and the fallback (if next_def != -1 else text[body_start:]) handles the last-function case.

Minor concerns:

  1. Hardcoded indentation string ("\n async def " = 8 spaces). This works because add_document_note_tool lives inside a method that nests it at that indent level. If someone reformats that file or refactors the nesting, this search will silently return a wider/narrower body than intended, without a test failure alerting to the breakage. A comment noting why 8 spaces are expected would add future-reader value (the PR already has an inline comment, which is good).

  2. Quote change in assertNotIn — the old search was 'requires the agent to be scoped to a corpus"' (note trailing "), clearly an attempt to match a string literal's closing quote. The new version drops that trailing quote. This is strictly more robust (won't break if the quote style changes), so the change is a net improvement.

  3. Approach fragility in general — this class of test (scanning source text rather than exercising runtime behavior) is inherently brittle. That said, it's the right trade-off when the behavior to guard is "does this function contain a rejected guard pattern", and there's nothing better to do here without a larger refactor.


Test Coverage

The PR description's test plan is minimal but appropriate — the tests themselves are the coverage. Both assertions in Fix 2 remain in place (the assertIn for the correct ternary and the assertNotIn for the guard string), so regression protection is preserved.


Summary

Both fixes correctly identify and address the actual test bugs. No production code is changed. The body-extraction logic in Fix 2 has acceptable fragility for this kind of source-scan test; a one-line comment about the 8-space indent (which the existing comment partially covers) would be the only improvement worth considering.

Approved to merge.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@JSv4 JSv4 merged commit c480e7e into main May 8, 2026
13 checks passed
@JSv4 JSv4 deleted the fix/pr1543-test-mock-targets branch May 8, 2026 05:40
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