[MPT-14912] Add e2e tests for billing journal sellers #188
Conversation
📝 WalkthroughWalkthroughTwo new end-to-end test modules are added for billing journal sellers—one async and one sync. Each module provides a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/e2e/billing/journal/seller/test_async_journal_seller.py:
- Around line 1-10: Add a short inline comment above the journal_sellers fixture
explaining that it reuses the seeded billing journal id from the test config;
reference the fixture name journal_sellers and the parameters async_mpt_vendor
and billing_journal_id so readers know this is a read-only seeded-resource
dependency and won’t create new journals if seeds change.
🧹 Nitpick comments (2)
tests/e2e/billing/journal/seller/test_sync_journal_seller.py (1)
21-31: Strengthen the assertion + avoid magic seeded seller name. (Based on learnings.)Recommend asserting the returned seller’s id equals
seller_id(and optionally name), and lifting"E2E Seeded Seller"into a constant with a short seed-data note. Also verify.select("-period")is supported here.tests/e2e/billing/journal/seller/test_async_journal_seller.py (1)
21-31: Strengthen assertion and extract seeded seller name constant.
- Add assertion for
result[0].id == seller_idto validate the returned seller matches expectations, not just count.- Extract
"E2E Seeded Seller"to a named constant at the top of the function; this string is duplicated across at least 4 test files (test_async_journal_seller.py, test_sync_journal_seller.py, test_async_sellers.py, test_sync_sellers.py), making it a good candidate for deduplication.Suggested changes
async def test_filter_journal_sellers(journal_sellers, seller_id): + SEEDED_SELLER_NAME = "E2E Seeded Seller" select_fields = ["-period"] filtered_sellers = ( journal_sellers.filter(RQLQuery(id=seller_id)) - .filter(RQLQuery(name="E2E Seeded Seller")) + .filter(RQLQuery(name=SEEDED_SELLER_NAME)) .select(*select_fields) ) result = [seller async for seller in filtered_sellers.iterate()] assert len(result) == 1 + assert result[0].id == seller_id
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/billing/journal/seller/test_async_journal_seller.pytests/e2e/billing/journal/seller/test_sync_journal_seller.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.pytests/e2e/billing/journal/seller/test_async_journal_seller.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.pytests/e2e/billing/journal/seller/test_async_journal_seller.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:13.551Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.pytests/e2e/billing/journal/seller/test_async_journal_seller.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/billing/journal/seller/test_sync_journal_seller.pytests/e2e/billing/journal/seller/test_async_journal_seller.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.pytests/e2e/billing/journal/seller/test_async_journal_seller.py
🧬 Code graph analysis (1)
tests/e2e/billing/journal/seller/test_async_journal_seller.py (3)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/billing/conftest.py (1)
billing_journal_id(17-18)mpt_api_client/models/model.py (1)
id(119-121)
⏰ 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 (3)
tests/e2e/billing/journal/seller/test_sync_journal_seller.py (2)
1-10: LGTM: sync companion coverage + consistentflakymarker.Fixture reuse for read-only calls aligns with existing e2e patterns. (Based on learnings.)
13-19: No issue found. The assertion is correct.
fetch_page()returns aCollectionobject, which properly implements the__len__()method (defined inmpt_api_client/models/collection.py). Therefore,len(result) > 0is a valid assertion and does not rely on incorrect assumptions about the return type being list-like.Likely an incorrect or invalid review comment.
tests/e2e/billing/journal/seller/test_async_journal_seller.py (1)
13-19: No issues found.fetch_page()returns aCollection[Model]instance, which implements__len__(), makinglen(result)a valid and correct assertion.Likely an incorrect or invalid review comment.
robcsegal
force-pushed
the
MPT-14912-add-e-2-e-tests-for-get-sellers-for-billing-journal
branch
from
8d69070 to
8605dd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/billing/journal/seller/test_sync_journal_seller.py (2)
15-20: Consider inlining the limit parameter.The
limitvariable is only used once and could be inlined for brevity, though the current implementation is perfectly acceptable.♻️ Optional refactor
def test_list_journal_sellers(journal_sellers): - limit = 10 - - result = journal_sellers.fetch_page(limit=limit) + result = journal_sellers.fetch_page(limit=10) assert len(result) > 0
23-33: Consider extracting the hardcoded seller name to a named constant for better maintainability.The test uses
seller_id(available from tests/e2e/conftest.py) and filters by the hardcoded string "E2E Seeded Seller". Extract this to a module-level constant to align with test maintainability practices when seed data changes:♻️ Optional refactor: extract constant
At the top of the file, add:
import pytest from mpt_api_client.rql.query_builder import RQLQuery + +E2E_SEEDED_SELLER_NAME = "E2E Seeded Seller" pytestmark = [pytest.mark.flaky]Then update the test:
def test_filter_journal_sellers(journal_sellers, seller_id): select_fields = ["-period"] filtered_sellers = ( journal_sellers.filter(RQLQuery(id=seller_id)) - .filter(RQLQuery(name="E2E Seeded Seller")) + .filter(RQLQuery(name=E2E_SEEDED_SELLER_NAME)) .select(*select_fields) ) result = list(filtered_sellers.iterate()) assert len(result) == 1
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/billing/journal/seller/test_async_journal_seller.pytests/e2e/billing/journal/seller/test_sync_journal_seller.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/billing/journal/seller/test_async_journal_seller.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:13.551Z
Learning: In the mpt-api-python-client E2E tests, the team intentionally reuses existing API resources (via e2e_config) for read-only operations and safe mutations (e.g., updating the "notes" field) to improve test execution speed. Only destructive/dangerous tests use isolated fixtures (created_price_list or async_created_price_list) that are created and cleaned up per test.
📚 Learning: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.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/billing/journal/seller/test_sync_journal_seller.py
📚 Learning: 2026-01-08T23:38:19.565Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 186
File: tests/e2e/billing/ledger/charge/test_sync_ledger_charge.py:33-39
Timestamp: 2026-01-08T23:38:19.565Z
Learning: In Python e2e tests under tests/e2e, hardcoded external IDs (e.g., INV12345) are intentional because they come from seeded test data. Ensure the test data seeds consistently include these IDs; if seeds change, update tests accordingly. Prefer using a named constant for such IDs (e.g., INV_EXTERNAL_ID) and document the dependency on seed data to avoid brittle tests.
Applied to files:
tests/e2e/billing/journal/seller/test_sync_journal_seller.py
⏰ 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 (3)
tests/e2e/billing/journal/seller/test_sync_journal_seller.py (3)
1-3: LGTM!The imports are necessary and properly used throughout the test module.
5-5: LGTM!The flaky marker is appropriate for e2e tests and aligns with the pytest configuration in pyproject.toml.
8-12: LGTM!The fixture follows best practices by reusing existing API resources for read-only operations, which speeds up test execution. The comment documenting the dependency on seeded data is excellent and aligns with the team's approach for e2e tests.
Based on learnings, this pattern is correct for read-only e2e tests.
|
d3rky
deleted the
MPT-14912-add-e-2-e-tests-for-get-sellers-for-billing-journal
branch
Add e2e tests for billing journal sellers
https://softwareone.atlassian.net/browse/MPT-14912
Closes MPT-14912
Release Notes
test_list_journal_sellers(sync & async) to verify listing sellers with pagination (limit=10) and non-empty resultstest_filter_journal_sellers(sync & async) to verify filtering by seller ID and name, selecting fields (excludesperiodvia "-period"), and asserting exactly one matchjournal_sellersfixture (sync & async variants) exposing the sellers resource for a given billing journal (relies on seeded e2e config)