feat: full assertion certainty for all plugins (v0.5.0)#4
Conversation
- Add AutoAssertError: raised immediately if mark_asserted() is called while record() is in progress, preventing auto-assert at runtime - Add _recording.py shared ContextVar module (avoids circular import between _base_plugin and _timeline) - BasePlugin.assertable_fields() is now a concrete default returning frozenset(interaction.details.keys()); no longer abstract - StateMachinePlugin._execute_step() accepts details= dict and return_interaction= flag; removes mark_asserted() call - SocketPlugin, DatabasePlugin, SmtpPlugin, PopenPlugin, AsyncWebSocketPlugin, SyncWebSocketPlugin: named per-step details dicts, typed assertion helpers (assert_connect, assert_send, etc.) - DatabasePlugin: new connect step; initial state disconnected - PopenPlugin: init renamed to spawn; _FakeStream retained as no-op - RedisPlugin: auto-assert removed; all three fields required; assert_command() typed helper added - SubprocessPlugin: run requires all four fields (command, returncode, stdout, stderr); which requires both fields (name, returns) - HttpPlugin: 5 fields expanded to 7; headers/body renamed to request_headers/request_body; response_headers and response_body added; HttpAssertionBuilder and assert_request() chained helper added - All tests updated with explicit assertions; 573 tests pass
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the assertion certainty across all plugins by enforcing explicit assertion of all recorded interaction fields and prohibiting automatic assertion. It introduces a new error type to catch auto-assertion anti-patterns at runtime and refactors several plugins to use named fields and dedicated assertion helpers, making tests more robust and explicit about expected interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Fix 13 ruff violations: sort imports (I001) in _timeline.py and test_timeline.py, remove bare f-string (F541) in popen_plugin.py, remove quoted annotations (UP037) in websocket_plugin.py and test_state_machine_plugin.py, and wrap long lines (E501) in http.py, popen_plugin.py, smtp_plugin.py, and socket_plugin.py.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and substantial pull request that significantly strengthens the core value proposition of the library by enforcing full assertion certainty. The removal of auto-asserting behavior across all plugins, the introduction of the AutoAssertError runtime guard, and the move to explicit, named fields for interactions are all fantastic improvements. The changes are consistent, well-documented, and thoroughly tested. I have one suggestion to improve the API clarity of the new HttpAssertionBuilder.
Note: Security Review did not run due to the size of the PR.
| def assert_request( | ||
| self, | ||
| method: str, | ||
| url: str, | ||
| headers: dict[str, Any] | None = None, | ||
| body: str = "", | ||
| ) -> "HttpAssertionBuilder": | ||
| """Return an HttpAssertionBuilder pre-loaded with expected request fields. | ||
|
|
||
| Call ``.assert_response()`` on the returned builder to complete the | ||
| assertion with all seven fields. | ||
| """ | ||
| return HttpAssertionBuilder( | ||
| verifier=self.verifier, | ||
| sentinel=self._sentinel, | ||
| method=method, | ||
| url=url, | ||
| headers=headers if headers is not None else {}, | ||
| body=body, | ||
| ) |
There was a problem hiding this comment.
The parameter names headers and body in assert_request and HttpAssertionBuilder.assert_response are ambiguous, as they refer to request attributes in one method and response attributes in the other. This is inconsistent with the recent renaming of HttpPlugin interaction fields to request_headers, request_body, etc.
For better clarity and consistency, I suggest renaming these parameters throughout the HttpAssertionBuilder flow:
- In
assert_request:headers->request_headers,body->request_body. - In
HttpAssertionBuilder.__init__: update to acceptrequest_headersandrequest_body. - In
HttpAssertionBuilder.assert_response:headers->response_headers,body->response_body.
This would make the chained API more intuitive. The example in README.md would also need to be updated to reflect this change.
| def assert_request( | |
| self, | |
| method: str, | |
| url: str, | |
| headers: dict[str, Any] | None = None, | |
| body: str = "", | |
| ) -> "HttpAssertionBuilder": | |
| """Return an HttpAssertionBuilder pre-loaded with expected request fields. | |
| Call ``.assert_response()`` on the returned builder to complete the | |
| assertion with all seven fields. | |
| """ | |
| return HttpAssertionBuilder( | |
| verifier=self.verifier, | |
| sentinel=self._sentinel, | |
| method=method, | |
| url=url, | |
| headers=headers if headers is not None else {}, | |
| body=body, | |
| ) | |
| def assert_request( | |
| self, | |
| method: str, | |
| url: str, | |
| request_headers: dict[str, Any] | None = None, | |
| request_body: str = "", | |
| ) -> "HttpAssertionBuilder": | |
| """Return an HttpAssertionBuilder pre-loaded with expected request fields. | |
| Call ``.assert_response()`` on the returned builder to complete the | |
| assertion with all seven fields. | |
| """ | |
| return HttpAssertionBuilder( | |
| verifier=self.verifier, | |
| sentinel=self._sentinel, | |
| method=method, | |
| url=url, | |
| request_headers=request_headers if request_headers is not None else {}, | |
| request_body=request_body, | |
| ) |
Rename HttpAssertionBuilder parameters to match the interaction field naming convention: headers -> request_headers, body -> request_body in assert_request(); headers -> response_headers, body -> response_body in assert_response(). Update tests and README example accordingly.
Revert the Gemini-suggested rename. assert_request(headers=, body=) and assert_response(status, headers=, body=) are unambiguous in context -- the method name already implies request/response scope. The prefixed form (request_headers=, response_body=) is redundant and breaks DRY.
1 participant
Summary
mark_asserted()duringrecord()raises immediately, preventing the auto-assert anti-pattern rather than silently passing testsheaders/bodyrenamed torequest_headers/request_body;response_headersandresponse_bodyadded;HttpAssertionBuilder+assert_request().assert_response()chained helperdetailsdicts replace generic{method, args, kwargs}; auto-assert removed; typed assertion helpers added on each proxyconnectstep recorded; initial state changed from"connected"to"disconnected""init"step renamed to"spawn"; stream ops removed;_FakeStreamretained as no-oprunfields required (command,returncode,stdout,stderr); bothwhichfields requiredassert_command()typed helper added@abstractmethodto concrete defaultfrozenset(interaction.details.keys())Breaking Changes
HttpPlugin:headers=→request_headers=,body=→request_body=; two new required fieldsSubprocessPlugin:runandwhichassertions now require all fieldsRedisPlugin: interactions no longer auto-asserted; explicit assertions requiredPopenPlugin: step"init"renamed to"spawn", stream step sentinels removedTest Plan
pytest tests/ -q)AutoAssertErrorraised immediately whenmark_asserted()called duringrecord()assert_interaction()call causes teardown failure for all plugins