Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Draft
jariy17 wants to merge 1 commit into
mainfrom
fix/unit-test-drift
Draft

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 wants to merge 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

@jariy17 jariy17 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks = registry._registered_callbacks.get(event_type, [])
assert all(asyncio.iscoroutinefunction(cb) for cb in callbacks)   # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

if not self.errorCode and self.label is None:      # value is never inspected
    raise ValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs

TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.

EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.

test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant