Skip to content

test: cover client context manager cleanup - #34

Merged
KhushiSharma22 merged 2 commits into
cyberwave-os:mainfrom
Bembaby:test/issue-29-client-context-manager
Oct 5, 2026
Merged

KhushiSharma22 merged 2 commits into
cyberwave-os:mainfrom
Bembaby:test/issue-29-client-context-manager

Conversation

@Bembaby

@Bembaby Bembaby commented Sep 25, 2026 •

Copy link
Copy Markdown

What changed and why?

Closes #29. The SDK's Cyberwave context manager had no direct offline coverage for cleanup on normal or exceptional exit. This adds two tests that check __enter__ returns the same client, disconnect is called once on both paths, and an exception raised inside the block propagates. The tests mock disconnect so they exercise the context-manager contract without initializing MQTT, a backend, or hardware. No SDK behavior or public API changes.

How did you test it?

On committed head 04e3a69f97dd64a972181a7fc5bcb6aeec1e933a with Python 3.13.15 and Poetry 2.4.3:

  • WSL Ubuntu: poetry run pytest tests/ -q — 3,031 passed, 109 skipped, 1 expected failure.
  • WSL Ubuntu: poetry run python tests/test_imports.py — 16/16 passed.
  • Windows: poetry run pytest tests/test_client_context_manager.py tests/test_client_twin.py -q — 16 passed.

On the original submitted commit, the Windows full suite had 7 failures, 3,024 passes, 109 skips, and 1 expected failure. I ran the same suite on the unchanged base in the same environment: it had the exact same seven failing test IDs and two fewer passes. On that commit, poetry run mypy cyberwave reported 722 errors in 104 package files, with identical error lines on the unchanged base; this check is not in the current CI workflow. The revised commit was validated with the Linux full suite and Windows focused tests listed above. No live service or robot was used.

Contributor checklist

  • Added focused tests for the requested behavior; production behavior is unchanged.
  • No documentation, public API, optional dependency, or simulation/live behavior changed.

AI assistance

AI assistance was used to prepare and review this test-only contribution and to run the listed checks. Personal human self-review is not claimed.

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Automated advisory review · Claude

Commit: 04e3a69f97dd64a972181a7fc5bcb6aeec1e933a. Human review and CI are still required.

The supplied patch adds a test file for the Cyberwave context-manager protocol. No production code changes are included, so this review is limited to the test logic itself. One concrete defect is visible in the patch excerpt.

  • Medium · tests/test_client_context_manager.py: In test_context_manager_calls_disconnect_and_propagates_block_error the disconnect.assert_called_once_with() assertion is placed outside and after the pytest.raises block, but it is also outside the patch.object context manager. Both patch.object calls use a 'with' block that ends before the outer assertions run. For the first test this happens to be fine because assert_called_once_with is inside the patch.object block. In the second test, however, disconnect.assert_called_once_with() is at the outermost indentation level, executed after the patch.object context has already exited and the mock has been torn down. Depending on the mock implementation this may pass vacuously or raise an AttributeError, meaning the assertion never actually guards the intended behavior. Fix: move disconnect.assert_called_once_with() inside the corresponding patch.object with-block, mirroring the structure used in the first test.

Coverage: 1 of 1 changed files; patch excerpts only. Generated REST code, lockfiles, notebooks, SVGs, missing patches and files beyond the 40-file / 60,000-character budget are excluded. GitHub may truncate individual patches.

@github-actions

Copy link
Copy Markdown

Thanks for contributing to the Cyberwave Python SDK, @Bembaby!

Maintainer contacts: @KhushiSharma22. When this PR is ready, we request a maintainer review and run an automated advisory review. CI and human review remain the source of truth.

Please describe what changed and how you tested it. See the contribution guide or ask for help on Discord. Drafts are welcome.

Merged human contributions receive a Discord thank-you. To opt out, check the PR template's Discord opt-out box or add <!-- cyberwave:no-celebration --> to the PR description.

@Bembaby

Bembaby commented Sep 25, 2026

Copy link
Copy Markdown
Author

Thanks for the advisory review. The revised tests mock disconnect directly, addressing the earlier concern about partially initialized client internals. On the current patch, both assert_called_once_with() calls run after patch.object restores the method, but the local disconnect mock still retains its call history; leaving the patch context does not reset that mock. The two tests use the same indentation and both assertions ran successfully in the focused and full Linux suites. I have kept the assertions as written so they check the completed context-manager exit.

@KhushiSharma22
KhushiSharma22 merged commit 81c9396 into cyberwave-os:main Oct 5, 2026
8 checks passed
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Thanks @Bembaby — your contribution has been merged!

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Slack team notifications

  • merged: sent

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.

Test Cyberwave client context-manager cleanup without network access

2 participants