MPT-14938 Add audit records e2e #173
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughAdds a top-level test configuration field, two pytest fixtures for audit record data and ID, and new synchronous and asynchronous end-to-end tests for audit record creation, retrieval, filtering, and not-found handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/e2e/audit/records/test_sync_records.py (1)
25-38: Add type hints for consistency.The parameters
audit_record_id(line 25) andproduct_id(line 32) are missing type hints. For consistency with other parameters in this file (e.g.,record_data: dict[str, Any]), these should be annotated asstr.🔎 Proposed fix
-def test_get_record(mpt_vendor: MPTClient, audit_record_id) -> None: +def test_get_record(mpt_vendor: MPTClient, audit_record_id: str) -> None: service = mpt_vendor.audit.records result = service.get(audit_record_id) assert result.id == audit_record_id -def test_iterate_records(mpt_vendor: MPTClient, product_id) -> None: +def test_iterate_records(mpt_vendor: MPTClient, product_id: str) -> None: service = mpt_vendor.audit.records.filter(RQLQuery(object__id=product_id)) records = list(service.iterate())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e_config.test.jsontests/e2e/audit/records/conftest.pytests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/test_sync_records.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/conftest.pytests/e2e/audit/records/test_sync_records.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/audit/records/conftest.py
🧬 Code graph analysis (1)
tests/e2e/audit/records/conftest.py (1)
tests/e2e/conftest.py (2)
account_id(122-123)e2e_config(89-92)
🪛 GitHub Actions: PR build and merge
tests/e2e/audit/records/test_async_records.py
[error] 1-1: Would reformat: tests/e2e/audit/records/test_async_records.py
tests/e2e/audit/records/conftest.py
[error] 1-1: Would reformat: tests/e2e/audit/records/conftest.py
tests/e2e/audit/records/test_sync_records.py
[error] 1-1: Would reformat: tests/e2e/audit/records/test_sync_records.py
🔇 Additional comments (7)
e2e_config.test.json (1)
14-14: LGTM!The new audit record configuration entry follows the established naming convention and ID format pattern used throughout the file.
tests/e2e/audit/records/conftest.py (1)
7-24: LGTM!The
record_datafixture provides a well-structured audit record payload with all necessary fields. The dependency onaccount_idfrom the parent conftest is correctly established.tests/e2e/audit/records/test_async_records.py (2)
1-10: LGTM!Imports are appropriate for async audit record testing, and the module-level
flakymarker is suitable for e2e tests that may have intermittent failures.
13-19: LGTM!The
created_recordasync fixture is properly implemented with correct use ofawaitfor the record creation operation.tests/e2e/audit/records/test_sync_records.py (3)
1-10: LGTM!Imports are appropriate for synchronous audit record testing, and the module-level
flakymarker is suitable for e2e tests.
13-17: LGTM!The
created_recordfixture is correctly implemented as a synchronous fixture for the sync client tests.
20-22: LGTM!The test correctly validates record creation by asserting the event and object ID match the input data.
albertsola
force-pushed
the
e2e/MPT-14938/audit-records-endpoints
branch
from
c277ecc to
191e43e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/e2e/audit/records/conftest.py (1)
23-25: LGTM with formatting fix needed.The
audit_record_idfixture correctly follows the same pattern as other ID extraction fixtures in the parent conftest.However, the pipeline indicates this file needs reformatting. Please run the formatter to resolve the formatting issue reported by the build.
tests/e2e/audit/records/test_async_records.py (1)
40-44: LGTM!The test correctly verifies that attempting to retrieve a non-existent record raises
MPTAPIError.However, note that the pipeline indicates this file needs reformatting. Please run the formatter to resolve the formatting issue.
tests/e2e/audit/records/test_sync_records.py (1)
41-45: LGTM with formatting fix needed.The test correctly verifies that attempting to retrieve a non-existent record raises
MPTAPIError.However, the pipeline indicates this file needs reformatting. Please run the formatter to resolve the formatting issue reported by the build.
🧹 Nitpick comments (2)
tests/e2e/audit/records/test_async_records.py (1)
19-21: Remove unused noqa directive.The
noqa: AAA01directive is flagged as unknown by the static analyzer. If this rule is not configured in your project's linter, consider removing the directive.🔎 Proposed fix
-def test_create_record(created_record: Record, record_data: dict[str, Any]) -> None: # noqa: AAA01 +def test_create_record(created_record: Record, record_data: dict[str, Any]) -> None: assert created_record.event == record_data["event"] assert created_record.object.id == record_data["object"]["id"]tests/e2e/audit/records/test_sync_records.py (1)
19-21: Remove unused noqa directive.The
noqa: AAA01directive is flagged as unknown by the static analyzer. If this rule is not configured in your project's linter, consider removing the directive.🔎 Proposed fix
-def test_create_record(created_record: Record, record_data: dict[str, Any]) -> None: # noqa: AAA01 +def test_create_record(created_record: Record, record_data: dict[str, Any]) -> None: assert created_record.event == record_data["event"] assert created_record.object.id == record_data["object"]["id"]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e_config.test.jsontests/e2e/audit/records/conftest.pytests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/test_sync_records.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/audit/records/conftest.py
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/audit/records/conftest.pytests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/test_sync_records.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/e2e/audit/records/test_async_records.py
🧬 Code graph analysis (3)
tests/e2e/audit/records/conftest.py (1)
tests/e2e/conftest.py (2)
account_id(122-123)e2e_config(89-92)
tests/e2e/audit/records/test_async_records.py (6)
mpt_api_client/mpt_client.py (1)
AsyncMPTClient(20-71)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/resources/audit/records.py (1)
Record(13-14)tests/e2e/conftest.py (2)
async_mpt_vendor(24-27)product_id(96-97)tests/e2e/audit/records/conftest.py (2)
record_data(7-20)audit_record_id(24-25)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/audit/records/test_sync_records.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/resources/audit/records.py (1)
Record(13-14)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
mpt_vendor(19-20)product_id(96-97)tests/e2e/audit/records/conftest.py (2)
record_data(7-20)audit_record_id(24-25)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.10)
tests/e2e/audit/records/test_async_records.py
19-19: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/audit/records/test_sync_records.py
19-19: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
e2e_config.test.json (1)
14-14: LGTM!The audit record ID configuration follows the established pattern and maintains alphabetical ordering.
tests/e2e/audit/records/conftest.py (1)
6-20: LGTM!The
record_datafixture provides well-structured test data for audit record creation, correctly utilizing theaccount_idfixture from the parent conftest.tests/e2e/audit/records/test_async_records.py (3)
13-16: LGTM!The async fixture correctly creates and returns an audit record using the vendor client.
24-28: LGTM!The test correctly retrieves an audit record by ID and validates the result asynchronously.
31-37: LGTM!The test correctly filters and iterates audit records using RQL, validating that the returned record matches the filter criteria.
tests/e2e/audit/records/test_sync_records.py (3)
13-16: LGTM!The sync fixture correctly creates and returns an audit record using the vendor client.
24-29: LGTM!The test correctly retrieves an audit record by ID and validates the result.
32-38: LGTM!The test correctly filters and iterates audit records using RQL, validating that the returned record matches the filter criteria.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.