OpenHands / OpenHands/software-agent-sdk
test_acp_agent.py flakes ~57% of runs: TestACPAgentCleanup counts other agents' _finalize calls
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 1.1k
- Forks
- 539
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 137
Description
Actual Behavior
tests/sdk/agent/test_acp_agent.py fails intermittently — 4 of 7 full-file runs on main at bf8a121d8:
uv run pytest tests/sdk/agent/test_acp_agent.py -q -p no:randomly
443 passed
443 passed
1 failed 442 passed
1 failed 442 passed
1 failed 442 passed
443 passed
1 failed 442 passed
The failing test moves between runs — observed as both TestACPAgentCleanup::test_atexit_cleanup_is_weak_and_inline and ::test_finalizer_falls_back_when_thread_start_fails — always with:
AssertionError: Expected '_finalize' to be called once. Called 2 times.
Calls: [call(), call()].
All 14 tests in the class pass in isolation (pytest tests/sdk/agent/test_acp_agent.py::TestACPAgentCleanup → 14 passed), which is what makes it look like flakiness rather than a defect.
Root cause. Two tests patch _finalize on the class and then assert a call count that is only meaningful per instance:
with patch.object(ACPAgent, "_finalize") as finalize:
...
finalize.assert_called_once_with()
- 48 sites in the file attach a
MagicMock()executor/connection to an agent and never close it, so_has_runtime_resources()stays true. - Those agents are freed at arbitrary later points in the run, not when their test ends.
ACPAgent.__del__then calls_finalizeon them.- With a class-level patch, that call lands on whichever mock is currently open.
Instrumenting __del__ confirms it fires with live resources during dozens of unrelated tests:
=== ACPAgent.__del__ fired WITH runtime resources, during: ===
3x during test: test_claude_provider_supports_runtime_switch
3x during test: test_resume_without_stored_cwd_still_works
3x during test: test_known_provider_surfaces_applied_override
2x during test: test_step_records_latency
...
which is why the failing test moves around.
Expected Behavior
The file passes deterministically. TestACPAgentCleanup's assertions should count only the _finalize calls made by their own agent.
Patching the instance is not an option — ACPAgent is a frozen pydantic model, so patch.object(agent, "_finalize") raises frozen_instance. Keeping the class patch but adding autospec=True records self, so each test can filter to its own calls:
OLD call_count = 2 -> assert_called_once_with() FAILS
NEW total = 2 own = 1 -> assertion PASSES
Acceptance Criteria
-
pytest tests/sdk/agent/test_acp_agent.pypasses on 10 consecutive full-file runs -
TestACPAgentCleanupstill asserts that its own agent's_finalizeran exactly once (the assertion is tightened, not weakened) - No change to
ACPAgentproduction code
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in tests/sdk/agent/test_acp_agent.py, especially TestACPAgentCleanup::test_atexit_cleanup_is_weak_and_inline and ::test_finalizer_falls_back_when_thread_start_fails. Review how the class-level _finalize patch records calls, then run the full file with pytest as described. Done means each test counts only its own agent's finalization, production code is unchanged, and ten consecutive full-file runs pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- testing-qa
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 88/100