feat(xtest): add audit cancel event tests#411
feat(xtest): add audit cancel event tests#411dmihalcik-virtru wants to merge 3 commits intoopentdf:mainfrom
Conversation
Bump pyright from 1.1.380 to 1.1.408 and alphabetize dev dependency ordering in pyproject.toml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Replace busy-wait polling with condition-based wakeup using threading.Condition. Improve clock skew handling with explicit timezone support. Add frozenset constants for audit types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Add test coverage for deferred/cancel audit event guarantees with 340-line test file covering cancel audit event scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Summary of ChangesHello @dmihalcik-virtru, 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 robustness and reliability of audit log testing by introducing new integration tests for deferred audit event guarantees, particularly focusing on scenarios involving client disconnections. It refines the underlying audit log collection mechanism for improved efficiency and clarity, ensuring that audit events are consistently captured and accurately reflect various operation outcomes. Additionally, it updates related test fixtures and dependencies to support these new testing capabilities. 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
|
|
![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 pull request introduces a comprehensive test suite for deferred audit event guarantees, particularly focusing on client cancellation scenarios. The changes in audit_logs.py to use threading.Condition for more efficient log waiting are a significant improvement. The new tests in test_audit_cancel.py are well-structured and provide good coverage for this critical feature.
My review includes a few suggestions to improve code quality and test clarity:
- Simplifying some redundant conditional logic in
audit_logs.py. - Aligning test names and docstrings with their implementations in
test_audit_logs_integration.pyto avoid confusion. - Adhering to PEP 8 for import statements.
Overall, this is a solid contribution that enhances the robustness of the audit logging tests.
| if collection_time.tzinfo is None: | ||
| # Assume local time, convert to UTC | ||
| collection_utc = collection_time.astimezone(UTC) | ||
| else: | ||
| collection_utc = collection_time.astimezone(UTC) |
There was a problem hiding this comment.
This if/else block is redundant as both branches execute the same code: collection_utc = collection_time.astimezone(UTC). The astimezone() method correctly handles both naive and timezone-aware datetime objects. You can simplify this by reverting to the single line of code that was here previously.
collection_utc = collection_time.astimezone(UTC)| if collection_t.tzinfo is None: | ||
| collection_utc = collection_t.astimezone(UTC) | ||
| else: | ||
| collection_utc = collection_t.astimezone(UTC) |
There was a problem hiding this comment.
Similar to a previous change, this if/else block is redundant. Both branches contain the same code, and astimezone() can handle both naive and aware datetime objects correctly. This can be simplified to a single line.
collection_utc = collection_t.astimezone(UTC)| def test_namespace_crud_audit( | ||
| self, otdfctl: OpentdfCommandLineTool, audit_logs: AuditLogAsserter | ||
| ): | ||
| """Test namespace creation audit trail.""" | ||
| """Test namespace create/update/delete audit trail.""" |
There was a problem hiding this comment.
The test is named test_namespace_crud_audit and the docstring mentions testing the 'create/update/delete audit trail', but the implementation only tests the 'create' part. This is misleading. To improve clarity, please either expand the test to cover update and delete operations, or rename it to test_namespace_create_audit to accurately reflect its scope.
| def test_attribute_crud_audit( | ||
| self, otdfctl: OpentdfCommandLineTool, audit_logs: AuditLogAsserter | ||
| ): | ||
| """Test attribute and value creation audit trail.""" |
There was a problem hiding this comment.
The test is named test_attribute_crud_audit, which implies it tests create, read, update, and delete operations for attributes. However, the implementation only covers the creation of an attribute and its values. Please consider expanding the test to cover other CRUD operations or rename it to test_attribute_create_audit for clarity.
| import base64 | ||
|
|
||
| h = pb.hash | ||
| altered = base64.b64encode(b"tampered" + base64.b64decode(h)[:8]) | ||
| pb.hash = str(altered) | ||
| else: | ||
| import base64 |
There was a problem hiding this comment.
The import base64 statements are placed inside the tamper_policy_binding function. According to PEP 8, imports should be at the top of the file. Please move these imports to the top of the file to adhere to Python's standard style guide. It seems import base64 was removed from the top of the file in this PR.
1 participant
Summary
test_audit_cancel.pywith 340 lines of test coverage for deferred/cancel audit event guaranteesParent PRs
fix/xtest-audit-framework)Test plan
cd xtest && uv run ruff check . && uv run pyrightuv run pytest test_audit_cancel.py --sdks go -v🤖 Generated with Claude Code
Part of stacked PR series decomposing
chore/the-claudiest-day-tmux