MPT-20436: Added endpoints and e2e tests for program certificates#319
Open
MPT-20436: Added endpoints and e2e tests for program certificates#319
Conversation
📝 WalkthroughWalkthroughThis PR introduces certificate management functionality to the MPT API client. It adds a new Certificate model and service classes (both sync and async variants) to manage program certificates, integrates them into the existing Program resource, and provides comprehensive unit and end-to-end test coverage for the new functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/e2e/program/certificate/test_sync_certificate.py (1)
22-25: Consider asserting resource identity/field changes, not only existence.Upgrading these checks will make failures more actionable and reduce false positives in E2E coverage.
Example assertion upgrades
def test_get_certificate_by_id(mpt_vendor, certificate_id): result = mpt_vendor.program.certificates.get(certificate_id) - assert result is not None + assert result is not None + assert result.id == certificate_id ... def test_update_certificate(mpt_client, created_certificate): @@ - assert result is not None + assert result is not None + assert result.id == created_certificate.id + assert result.name == updated_name ... def test_terminate_certificate( @@ - assert result is not None + assert result is not None + assert result.id == created_certificate.idAlso applies to: 61-67, 70-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/certificate/test_sync_certificate.py` around lines 22 - 25, The test currently only asserts non-None for test_get_certificate_by_id; change it to assert concrete fields/identity from the retrieved object (e.g., that result.id == certificate_id and other important fields like result.status, result.owner or result.updated_at match expected values or types) so failures are actionable; update the same pattern in the other certificate tests referenced (the blocks around lines 61-67 and 70-79) that call mpt_vendor.program.certificates.get/create/list to validate key fields and identities rather than only existence.tests/unit/resources/program/test_certificates.py (1)
51-69: Prefer semantic JSON assertions over raw-byte equality in request checks.The current byte-level check is brittle to harmless serialization changes. Compare decoded JSON payloads instead.
Proposed refactor
+import json ... - request_expected_content = b'{"id":"CER-123","status":"updated"}' ... - assert request.content == request_expected_content + assert json.loads(request.content) == input_status ... - request_expected_content = b'{"id":"CER-123","status":"updated"}' ... - assert request.content == request_expected_content + assert json.loads(request.content) == input_statusAlso applies to: 80-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/test_certificates.py` around lines 51 - 69, Replace the brittle byte-level assertion of request.content with a semantic JSON comparison: decode the request body (e.g., via json.loads(request.content) or request.json()) and assert the resulting dict equals the expected payload dict (use a variable like request_expected_data instead of request_expected_content). Update the assertion in the certificate_service.terminate test (replace assert request.content == request_expected_content) and make the same change for the similar assertions referenced at the other occurrence (lines around 80-98) to compare decoded JSON dicts instead of raw bytes.tests/e2e/program/certificate/test_async_certificate.py (1)
26-30: Consider strengthening assertions beyond non-null checks.These tests would catch regressions earlier if they validated returned identifiers/updated fields/status transitions instead of only existence.
Example assertion upgrades
async def test_get_certificate_by_id(async_mpt_vendor, certificate_id): result = await async_mpt_vendor.program.certificates.get(certificate_id) - assert result is not None + assert result is not None + assert result.id == certificate_id ... async def test_update_certificate(async_mpt_client, created_certificate): @@ - assert result is not None + assert result is not None + assert result.id == created_certificate.id + assert result.name == updated_name ... async def test_terminate_certificate( @@ - assert result is not None + assert result is not None + assert result.id == created_certificate.idAlso applies to: 65-71, 74-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/certificate/test_async_certificate.py` around lines 26 - 30, The test test_get_certificate_by_id currently only asserts non-null; update it to validate key certificate fields to catch regressions: assert that result.id equals the input certificate_id, assert expected status (e.g., "issued" or the known default/state transition for this fixture), and assert important fields like result.owner_id or result.updated_at/created_at are present and well-formed (not None/empty). Apply the same pattern to the other certificate tests referenced (the blocks around lines 65-71 and 74-83) by replacing bare existence checks with explicit assertions of identifiers, status transitions, and at least one changed/updated field to ensure the API returns correct data. Ensure you reference the async_mpt_vendor.program.certificates.get call and the certificate_id fixture when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/program/certificate/test_async_certificate.py`:
- Around line 26-30: The test test_get_certificate_by_id currently only asserts
non-null; update it to validate key certificate fields to catch regressions:
assert that result.id equals the input certificate_id, assert expected status
(e.g., "issued" or the known default/state transition for this fixture), and
assert important fields like result.owner_id or result.updated_at/created_at are
present and well-formed (not None/empty). Apply the same pattern to the other
certificate tests referenced (the blocks around lines 65-71 and 74-83) by
replacing bare existence checks with explicit assertions of identifiers, status
transitions, and at least one changed/updated field to ensure the API returns
correct data. Ensure you reference the async_mpt_vendor.program.certificates.get
call and the certificate_id fixture when adding these assertions.
In `@tests/e2e/program/certificate/test_sync_certificate.py`:
- Around line 22-25: The test currently only asserts non-None for
test_get_certificate_by_id; change it to assert concrete fields/identity from
the retrieved object (e.g., that result.id == certificate_id and other important
fields like result.status, result.owner or result.updated_at match expected
values or types) so failures are actionable; update the same pattern in the
other certificate tests referenced (the blocks around lines 61-67 and 70-79)
that call mpt_vendor.program.certificates.get/create/list to validate key fields
and identities rather than only existence.
In `@tests/unit/resources/program/test_certificates.py`:
- Around line 51-69: Replace the brittle byte-level assertion of request.content
with a semantic JSON comparison: decode the request body (e.g., via
json.loads(request.content) or request.json()) and assert the resulting dict
equals the expected payload dict (use a variable like request_expected_data
instead of request_expected_content). Update the assertion in the
certificate_service.terminate test (replace assert request.content ==
request_expected_content) and make the same change for the similar assertions
referenced at the other occurrence (lines around 80-98) to compare decoded JSON
dicts instead of raw bytes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d5571e80-e555-40b7-be6a-5314e4367d28
📒 Files selected for processing (8)
e2e_config.test.jsonmpt_api_client/resources/program/certificates.pympt_api_client/resources/program/program.pytests/e2e/program/certificate/conftest.pytests/e2e/program/certificate/test_async_certificate.pytests/e2e/program/certificate/test_sync_certificate.pytests/unit/resources/program/test_certificates.pytests/unit/resources/program/test_program.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds support for program certificates in the API client, including synchronous and asynchronous service classes and comprehensive end-to-end and unit test coverage.
Program Certificate Service Implementation:
Certificatemodel andCertificateService/AsyncCertificateServiceclasses inmpt_api_client/resources/program/certificates.py, providing CRUD operations viaManagedResourceMixinandCollectionMixin, along with aterminateaction method for both sync and async services.Integration with Program Module:
certificatesas a property onProgramandAsyncPrograminmpt_api_client/resources/program/program.py, exposing the new service alongside the existing enrollments service.Testing Enhancements:
tests/e2e/program/certificate/test_sync_certificate.pyandtest_async_certificate.py, covering creation, retrieval, listing, updating, termination, and error handling.tests/e2e/program/certificate/conftest.py.CertificateService,AsyncCertificateService, and theProgram/AsyncProgramintegration intests/unit/resources/program/test_certificates.pyandtests/unit/resources/program/test_program.py.Configuration Updates:
e2e_config.test.jsonwithprogram.certificate.idfor E2E testing.Closes MPT-20436