[MPT-14910] Add e2e tests for some billing journals endpoints #172
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughRemoved the standalone journal_upload resource and moved multipart upload capability into the existing JournalsService/AsyncJournalsService; added e2e config/test data, new fixtures, and sync/async E2E and unit tests for journal CRUD and upload flows. Changes
Sequence Diagram(s)%%{init: {"theme":"base","themeVariables":{"primaryColor":"#E6FFFA","actorBorder":"#2B6CB0","actorBackground":"#EBF8FF","noteBackground":"#F7FAFC","lineColor":"#A0AEC0"}}}%%
sequenceDiagram
participant Client
participant JournalsService as "JournalsService / Async"
participant BillingAPI as "Billing API"
participant Model as "Journal Model"
Client->>JournalsService: upload(journal_id, file?)
alt file provided
JournalsService->>JournalsService: build multipart payload\n(files: {file_key: file}, data: {data_key: id})
else no file
JournalsService->>JournalsService: build form payload\n(data: {data_key: id})
end
JournalsService->>BillingAPI: POST /public/v1/billing/journals/{id}/upload (multipart)
BillingAPI-->>JournalsService: 200 + response body
JournalsService->>Model: _model_class.from_response(response)
Model-->>Client: Journal instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
robcsegal
force-pushed
the
MPT-14910-add-e-2-e-tests-for-billing-journals
branch
from
48028f7 to
5b0512b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/e2e/billing/journal/test_sync_journal.py (2)
19-22: Remove unusednoqadirective.Per static analysis, the
# noqa: WPS421directive is not recognized/needed by the current linter configuration.🔎 Proposed fix
- print(f"TEARDOWN - Unable to delete billing journal: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete billing journal: {error.title}")
37-41: Unused fixture:completed_billing_journal.This fixture is defined but not used by any test in this module. Consider removing it or adding a test that exercises the complete workflow.
tests/e2e/billing/journal/test_async_journal.py (2)
21-24: Remove unusednoqadirective.Per static analysis, the
# noqa: WPS421directive is not recognized/needed by the current linter configuration.🔎 Proposed fix
- print(f"TEARDOWN - Unable to delete billing journal: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete billing journal: {error.title}")
39-43: Unused fixture:completed_billing_journal.This fixture is defined but not used by any test in this module. Consider removing it or adding a test that exercises the complete workflow.
mpt_api_client/resources/billing/journal_upload.py (3)
26-26: Remove unusednoqadirective.Per static analysis, the
# noqa: WPS110directive is not recognized/needed by the current linter configuration.🔎 Proposed fix
- def upload(self, resource_data: str, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def upload(self, resource_data: str, file: FileTypes | None = None) -> Model:
60-60: Remove unusednoqadirective.Per static analysis, the
# noqa: WPS110directive is not recognized/needed by the current linter configuration.🔎 Proposed fix
- async def upload(self, resource_data: str, file: FileTypes | None = None) -> Model: # noqa: WPS110 + async def upload(self, resource_data: str, file: FileTypes | None = None) -> Model:
26-51: Consider more specific return type annotation.The return type is annotated as
Modelbut could beJournalUploadfor better type safety and IDE support since_model_classis set toJournalUploadin the config.🔎 Proposed fix
- def upload(self, resource_data: str, file: FileTypes | None = None) -> Model: + def upload(self, resource_data: str, file: FileTypes | None = None) -> JournalUpload:- async def upload(self, resource_data: str, file: FileTypes | None = None) -> Model: + async def upload(self, resource_data: str, file: FileTypes | None = None) -> JournalUpload:Also applies to: 60-85
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/test_billing_journal.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (8)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_upload.pytests/data/test_billing_journal.jsonltests/e2e/billing/conftest.pytests/e2e/billing/journal/conftest.pytests/e2e/billing/journal/test_async_journal.pytests/e2e/billing/journal/test_sync_journal.pytests/unit/resources/billing/test_journal_upload.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/billing/journal/conftest.pytests/unit/resources/billing/test_journal_upload.pytests/e2e/billing/conftest.pytests/e2e/billing/journal/test_async_journal.pytests/e2e/billing/journal/test_sync_journal.py
🧬 Code graph analysis (5)
tests/unit/resources/billing/test_journal_upload.py (2)
mpt_api_client/http/types.py (2)
Response(27-42)json(40-42)mpt_api_client/resources/billing/journal_upload.py (2)
upload(26-51)upload(60-85)
tests/e2e/billing/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/journal/test_async_journal.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/billing/journal/conftest.py (1)
billing_journal_factory(10-21)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/billing/conftest.py (1)
billing_journal_fd(7-9)mpt_api_client/resources/billing/journal_upload.py (2)
upload(26-51)upload(60-85)
tests/e2e/billing/journal/test_sync_journal.py (7)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/billing/journal/conftest.py (1)
billing_journal_factory(10-21)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/billing/conftest.py (1)
billing_journal_fd(7-9)mpt_api_client/resources/billing/journal_upload.py (2)
upload(26-51)upload(60-85)
mpt_api_client/resources/billing/journal_upload.py (7)
mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/resources/billing/journals.py (2)
upload(68-72)upload(103-107)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/async_client.py (1)
request(61-117)mpt_api_client/http/client.py (1)
request(73-129)mpt_api_client/http/base_service.py (1)
path(28-30)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
🪛 Ruff (0.14.10)
tests/e2e/billing/journal/test_async_journal.py
24-24: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/test_sync_journal.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/billing/journal_upload.py
26-26: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
60-60: Unused noqa directive (unknown: WPS110)
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 (9)
e2e_config.test.json (1)
14-14: LGTM!The new
billing.journal.idkey follows the established naming convention and ID format pattern used throughout the configuration file.tests/e2e/billing/conftest.py (1)
12-14: LGTM!The fixture correctly retrieves the billing journal ID from the e2e configuration.
tests/e2e/billing/journal/test_sync_journal.py (1)
44-108: LGTM!Good coverage of CRUD operations, filtering, and upload functionality. The test structure is clear and follows consistent patterns with proper use of fixtures.
tests/e2e/billing/journal/conftest.py (1)
4-21: LGTM!The fixtures are well-structured. The factory pattern allows flexible creation of billing journal payloads with customizable names while providing sensible defaults.
tests/e2e/billing/journal/test_async_journal.py (2)
78-81: Correct use of synchronous test function.Good practice: declaring
test_create_billing_journalasdef(synchronous) since it only uses async fixtures without anyawaitcalls. pytest-asyncio will resolve the async fixtures automatically. Based on learnings from previous reviews.
46-114: LGTM!The async test implementations properly mirror the sync versions with correct
awaitusage throughout.tests/unit/resources/billing/test_journal_upload.py (2)
37-48: LGTM!The parametrized tests correctly verify that the
uploadmethod is present on both sync and async service classes, reflecting the API change fromcreatetoupload.
51-112: LGTM!Comprehensive test coverage for the upload functionality:
- Both sync and async variants tested
- File upload and no-file scenarios covered
- Mock routes verify the correct endpoint is called
- Good use of
tmp_pathfixture for temporary file creationmpt_api_client/resources/billing/journal_upload.py (1)
36-47: LGTM!The upload implementation correctly constructs the multipart payload when a file is provided and delegates to the HTTP client with
force_multipart=True. The pattern is consistent between sync and async variants.Also applies to: 70-81
robcsegal
force-pushed
the
MPT-14910-add-e-2-e-tests-for-billing-journals
branch
from
5b0512b to
4db7beb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/resources/billing/test_journal_upload.py (1)
51-112: Consider adding more specific assertions for upload tests.While the current assertions verify that the endpoint is called and returns a result, consider adding assertions to verify:
- The request payload structure (multipart form data with correct keys)
- The response content is properly parsed into the expected model
This would provide more confidence that the upload method works correctly beyond just calling the endpoint.
💡 Example enhancement
def test_upload(journal_upload_service, tmp_path) -> None: file_path = tmp_path / "journal.jsonl" file_path.write_text("test data") with file_path.open("rb") as file_obj, respx.mock: mock_route = respx.post( "https://api.example.com/public/v1/billing/journals/JRN-0000-0001/upload" ).mock(return_value=httpx.Response(200, json={"id": "JRN-0000-0001", "status": "uploaded"})) result = journal_upload_service.upload( resource_data="JRN-0000-0001", file=file_obj, ) assert mock_route.called assert result is not None # Verify multipart structure was sent request = mock_route.calls.last.request assert "multipart/form-data" in request.headers.get("content-type", "")tests/e2e/billing/journal/test_async_journal.py (1)
104-113: Consider more specific assertions for the upload test.While the test verifies the upload completes successfully, consider adding assertions to verify:
- The journal state after upload (if the API returns status)
- The presence of expected fields in the response
- Or at minimum, check the result has an
idattribute💡 Example enhancement
async def test_upload_billing_journal( async_mpt_vendor, created_billing_journal, billing_journal_fd ): billing_journal_file_data = created_billing_journal.id result = await async_mpt_vendor.billing.journals.upload(created_billing_journal.id).upload( resource_data=billing_journal_file_data, file=billing_journal_fd, ) assert result is not None + assert hasattr(result, 'id') # Verify response structure
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/test_billing_journal.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (8)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_upload.pytests/data/test_billing_journal.jsonltests/e2e/billing/conftest.pytests/e2e/billing/journal/conftest.pytests/e2e/billing/journal/test_async_journal.pytests/e2e/billing/journal/test_sync_journal.pytests/unit/resources/billing/test_journal_upload.py
✅ Files skipped from review due to trivial changes (1)
- tests/data/test_billing_journal.jsonl
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e_config.test.json
- tests/e2e/billing/journal/conftest.py
- tests/e2e/billing/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/unit/resources/billing/test_journal_upload.pytests/e2e/billing/journal/test_async_journal.pytests/e2e/billing/journal/test_sync_journal.py
🧬 Code graph analysis (2)
tests/unit/resources/billing/test_journal_upload.py (1)
mpt_api_client/resources/billing/journal_upload.py (2)
upload(26-51)upload(60-85)
mpt_api_client/resources/billing/journal_upload.py (4)
mpt_api_client/resources/billing/journals.py (2)
upload(68-72)upload(103-107)mpt_api_client/http/async_client.py (1)
request(61-117)mpt_api_client/http/client.py (1)
request(73-129)mpt_api_client/http/base_service.py (1)
path(28-30)
🪛 Ruff (0.14.10)
tests/e2e/billing/journal/test_async_journal.py
24-24: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/billing/journal_upload.py
26-26: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
60-60: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/test_sync_journal.py
22-22: Unused noqa directive (unknown: WPS421)
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 (9)
tests/unit/resources/billing/test_journal_upload.py (1)
1-49: LGTM! Test structure and coverage are solid.The unit tests comprehensively verify:
- Endpoint path construction
- Method presence (upload)
- Both synchronous and asynchronous upload flows
- Upload with and without file scenarios
The test structure is clean and follows established patterns in the codebase.
tests/e2e/billing/journal/test_async_journal.py (4)
9-25: LGTM! Fixture properly handles creation and teardown.The fixture correctly:
- Creates a billing journal for testing
- Yields it for test usage
- Handles teardown with error handling to prevent test failures from cleanup issues
The
27-36: LGTM! Fixture correctly exercises the new upload interface.The fixture properly demonstrates the new upload workflow:
- Submits the journal to change its state
- Uses the new chained upload interface:
journals.upload(id).upload(data, file)This aligns with the implementation changes in
JournalUploadService.
78-81: LGTM! Correct use of synchronous test function with async fixture.The test correctly uses
definstead ofasync defsince it only needs the resolved fixture value without performing any async operations. This follows the established pattern for tests that consume async fixtures without awaiting.Based on learnings, this is the preferred approach to avoid unnecessary async function declarations.
24-24: Thenoqa: WPS421directive is correct and should be kept.The repository uses wemake-python-styleguide, which enforces WPS421 (print statement usage). The
noqacomment properly suppresses this warning for the print statement used in the teardown error handling.mpt_api_client/resources/billing/journal_upload.py (2)
10-17: LGTM! Configuration is well-structured.The service configuration clearly defines:
- Endpoint template with journal_id parameter
- Model class for response parsing
- Upload keys for multipart form data
26-26: Verify if wemake-python-styleguide linter is in use.Similar to the test files, the
noqa: WPS110directive is for wemake-python-styleguide. If this linter is not configured in your project, these directives can be removed to eliminate Ruff warnings.(Same verification script as in the async test file comment)
tests/e2e/billing/journal/test_sync_journal.py (2)
1-107: LGTM! Synchronous test suite mirrors async version well.The test structure is solid and provides comprehensive coverage:
- CRUD operations (create, read, update, delete)
- Filtering and pagination
- Upload functionality
- Error handling (404 not found)
- Proper teardown with error handling
The tests follow established patterns and are well-organized.
22-22: Verify if wemake-python-styleguide linter is in use.Same as the async test file - the
noqa: WPS421directive is for wemake-python-styleguide. If this linter is not configured in your project, consider removing these directives.(Refer to the same verification script provided for the async test file)
robcsegal
force-pushed
the
MPT-14910-add-e-2-e-tests-for-billing-journals
branch
from
4db7beb to
94c0444
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
mpt_api_client/resources/billing/journal_upload.py (1)
36-51: Critical:resource_datais silently dropped whenfile=None.When no file is provided,
resource_datais never added to thefilesdict (it's only added inside theif file:block on lines 39-40). The API receives an empty payload instead of the expected data.This was flagged in previous reviews but remains unaddressed. Current e2e tests don't catch this because they always pass a file.
🔎 Proposed fix
Move
resource_dataassignment outside the conditional:def upload(self, resource_data: str, file: FileTypes | None = None) -> Model: # noqa: WPS110 """Upload journal file. Args: resource_data: Resource data. file: File image. Returns: Model: Created resource. """ files = {} + files[self._upload_data_key] = resource_data # type: ignore if file: files[self._upload_file_key] = file # type: ignore[attr-defined] - files[self._upload_data_key] = resource_data # type: ignore response = self.http_client.request( # type: ignore[attr-defined] "post", self.path, # type: ignore[attr-defined] files=files, force_multipart=True, ) return self._model_class.from_response( response ) # type: ignore[attr-defined, no-any-return]Apply the same fix to the async version at lines 70-85.
🧹 Nitpick comments (2)
tests/e2e/billing/journal/test_async_journal.py (1)
104-113: Consider adding a test case for upload without a file.All current upload tests pass a file. Adding a test with
file=Nonewould verify the API's behavior and catch the critical bug flagged injournal_upload.pywhereresource_datais dropped when no file is provided.Example test case
async def test_upload_billing_journal_without_file( async_mpt_vendor, created_billing_journal ): billing_journal_file_data = created_billing_journal.id result = await async_mpt_vendor.billing.journals.upload( created_billing_journal.id ).upload( resource_data=billing_journal_file_data, file=None, ) assert result is not Nonetests/e2e/billing/journal/test_sync_journal.py (1)
100-108: Consider adding a test case for upload without a file.All current upload tests pass a file. Adding a test with
file=Nonewould verify the API's behavior and catch the critical bug flagged injournal_upload.pywhereresource_datais dropped when no file is provided.Example test case
def test_upload_billing_journal_without_file( mpt_vendor, created_billing_journal ): billing_journal_file_data = created_billing_journal.id result = mpt_vendor.billing.journals.upload( created_billing_journal.id ).upload( resource_data=billing_journal_file_data, file=None, ) assert result is not None
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/test_billing_journal.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (8)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_upload.pytests/data/test_billing_journal.jsonltests/e2e/billing/conftest.pytests/e2e/billing/journal/conftest.pytests/e2e/billing/journal/test_async_journal.pytests/e2e/billing/journal/test_sync_journal.pytests/unit/resources/billing/test_journal_upload.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/billing/conftest.py
- tests/e2e/billing/journal/conftest.py
- tests/unit/resources/billing/test_journal_upload.py
- tests/data/test_billing_journal.jsonl
🧰 Additional context used
🧠 Learnings (1)
📚 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/billing/journal/test_sync_journal.pytests/e2e/billing/journal/test_async_journal.py
🧬 Code graph analysis (3)
mpt_api_client/resources/billing/journal_upload.py (5)
mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/resources/billing/journals.py (2)
upload(68-72)upload(103-107)mpt_api_client/http/async_client.py (1)
request(61-117)mpt_api_client/http/client.py (1)
request(73-129)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
tests/e2e/billing/journal/test_sync_journal.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/billing/journal/conftest.py (1)
billing_journal_factory(10-21)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/billing/journal/test_async_journal.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/billing/journal_upload.py (2)
upload(26-51)upload(60-85)
🪛 Ruff (0.14.10)
mpt_api_client/resources/billing/journal_upload.py
26-26: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
60-60: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/test_sync_journal.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/test_async_journal.py
24-24: Unused noqa directive (unknown: WPS421)
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 (2)
e2e_config.test.json (1)
14-14: LGTM!Addition of the billing journal test ID aligns with the new e2e test coverage.
mpt_api_client/resources/billing/journal_upload.py (1)
26-26: Remove unusednoqadirectives.The
WPS110check is not recognized by Ruff.🔎 Proposed fix
- def upload(self, resource_data: str, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def upload(self, resource_data: str, file: FileTypes | None = None) -> Model:Apply the same change to line 60 for the async version.
Also applies to: 60-60
⛔ Skipped due to learnings
Learnt from: jentyk Repo: softwareone-platform/mpt-api-python-client PR: 136 File: tests/e2e/notifications/subscribers/conftest.py:14-25 Timestamp: 2025-12-01T10:48:52.586Z Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
robcsegal
force-pushed
the
MPT-14910-add-e-2-e-tests-for-billing-journals
branch
from
94c0444 to
832937c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/resources/billing/test_journals.py (1)
80-127: LGTM! Upload tests provide good coverage.The unit tests appropriately cover:
- Synchronous and asynchronous upload with files
- Upload without files (file=None case)
- HTTP mocking with respx
Optional: Consider adding assertions to verify the multipart payload structure in the mocked request. For example:
assert mock_route.calls.last.request.headers.get("content-type").startswith("multipart/form-data") # Verify file was included in the requestThis would help catch issues with how the multipart payload is constructed.
mpt_api_client/resources/billing/journals.py (1)
60-72: Consider refactoring the multipart payload construction for clarity and convention.The current implementation works (tests confirm), but has style concerns:
Line 64: Passing
journal_id(a string) in thefilesdict is unconventional. While HTTPX accepts string values as form fields, form data should typically use adataparameter instead. Verify API requirements and adjust accordingly.Line 66:
urljoin(f"{self.path}/", f"{journal_id}/upload")is unnecessarily complex. Usef"{self.path}/{journal_id}/upload"instead.Lines 60-72: When
fileisNone, an emptyfilesdict is passed withforce_multipart=True, resulting in an empty multipart request. This works but is an unusual pattern—consider whether this behavior is intentional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/test_billing_journal.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (10)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_upload.pympt_api_client/resources/billing/journals.pytests/data/test_billing_journal.jsonltests/e2e/billing/conftest.pytests/e2e/billing/journal/conftest.pytests/e2e/billing/journal/test_async_journal.pytests/e2e/billing/journal/test_sync_journal.pytests/unit/resources/billing/test_journal_upload.pytests/unit/resources/billing/test_journals.py
💤 Files with no reviewable changes (2)
- mpt_api_client/resources/billing/journal_upload.py
- tests/unit/resources/billing/test_journal_upload.py
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/billing/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/data/test_billing_journal.jsonl
- e2e_config.test.json
- tests/e2e/billing/journal/conftest.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/unit/resources/billing/test_journals.pytests/e2e/billing/journal/test_sync_journal.pytests/e2e/billing/journal/test_async_journal.py
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
tests/e2e/billing/journal/test_sync_journal.pytests/e2e/billing/journal/test_async_journal.py
🧬 Code graph analysis (4)
mpt_api_client/resources/billing/journals.py (3)
mpt_api_client/http/async_service.py (1)
AsyncService(12-80)mpt_api_client/resources/billing/custom_ledgers.py (2)
upload(52-57)upload(83-88)mpt_api_client/http/base_service.py (1)
path(28-30)
tests/unit/resources/billing/test_journals.py (2)
mpt_api_client/http/types.py (2)
Response(27-42)json(40-42)mpt_api_client/resources/billing/journals.py (2)
upload(50-77)upload(108-135)
tests/e2e/billing/journal/test_sync_journal.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/billing/journal/conftest.py (1)
billing_journal_factory(10-21)tests/e2e/billing/conftest.py (1)
billing_journal_fd(7-13)mpt_api_client/resources/billing/journals.py (2)
upload(50-77)upload(108-135)
tests/e2e/billing/journal/test_async_journal.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/billing/journals.py (2)
upload(50-77)upload(108-135)
🪛 Ruff (0.14.10)
mpt_api_client/resources/billing/journals.py
50-50: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
108-108: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/test_sync_journal.py
22-22: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/test_async_journal.py
24-24: Unused noqa directive (unknown: WPS421)
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 (7)
tests/e2e/billing/journal/test_async_journal.py (4)
9-25: LGTM! Fixture properly manages billing journal lifecycle.The
created_billing_journalfixture correctly:
- Creates a journal via the API
- Yields it for test use
- Cleans up with proper error handling in teardown
27-35: Verify the upload call in the submitted fixture.The fixture calls
async_mpt_vendor.billing.journals.upload()on lines 30-33. Given the concerns raised about the upload method implementation (specifically addingjournal_idto thefilesdict), please verify this works correctly in the e2e environment.Once the upload method implementation is confirmed or fixed, this fixture should work as expected.
45-111: Comprehensive test coverage for journal operations.The test suite covers all essential CRUD operations plus upload functionality:
- Get by ID (valid and invalid)
- List with pagination
- Filter with RQL queries
- Create, update, delete
- Upload file
Test assertions are appropriate and verify expected behavior.
77-80: Correct use of sync test function with async fixture.This follows the recommended pattern: when a test uses async fixtures but has no await statements, declare it as
def(synchronous) rather thanasync def. Pytest-asyncio will resolve the async fixtures automatically.Based on learnings, this is the correct approach to avoid linter warnings about unnecessary async functions.
tests/e2e/billing/journal/test_sync_journal.py (3)
9-23: LGTM! Sync fixture mirrors async version correctly.The fixture properly manages the billing journal lifecycle for synchronous tests, with appropriate teardown error handling.
25-33: Verify the upload call in the submitted fixture.Similar to the async version, this fixture calls the upload method on lines 28-31. Please verify this works correctly with the current upload implementation, particularly regarding the multipart payload structure.
43-105: Comprehensive sync test coverage.The synchronous test suite provides excellent coverage of all journal operations:
- CRUD operations (create, read, update, delete)
- Listing and filtering
- Error handling (404 Not Found)
- File upload
All tests use appropriate synchronous patterns with the sync MPT client.
|
albertsola
deleted the
MPT-14910-add-e-2-e-tests-for-billing-journals
branch
Add e2e tests for some billing journals endpoints
https://softwareone.atlassian.net/browse/MPT-14910
Summary by CodeRabbit
New Features
Removed
Tests
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.