[MPT-14926] Added e2e tests for billing overrides #190
Conversation
📝 WalkthroughWalkthroughAdds a billing override identifier to test configuration and introduces fixtures plus parallel synchronous and asynchronous end-to-end tests for billing override operations (get, list, not-found, filtering, field selection, pagination). Tests are marked flaky. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*⚙️ CodeRabbit configuration file
Files:
**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-01-08T08:34:05.465ZApplied to files:
📚 Learning: 2026-01-08T23:38:19.565ZApplied to files:
📚 Learning: 2025-12-12T15:02:20.732ZApplied to files:
🧬 Code graph analysis (1)tests/e2e/billing/override/test_sync_override.py (5)
⏰ 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)
🔇 Additional comments (9)
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: 0
🧹 Nitpick comments (4)
tests/e2e/billing/override/test_async_override.py (2)
9-12: Consider strengthening the assertion.The test only verifies that
result is not None. Consider also asserting thatresult.id == billing_override_idto confirm the correct resource was retrieved.💡 Suggested enhancement
async def test_get_billing_override_by_id(async_mpt_ops, billing_override_id): result = await async_mpt_ops.billing.manual_overrides.get(billing_override_id) assert result is not None + assert result.id == billing_override_id
28-38: Extract hardcoded externalId to a named constant.Line 32 uses the hardcoded string
"e2e-seeded-override". Based on learnings, hardcoded external IDs in e2e tests are intentional due to seeded test data, but should be extracted to a named constant (e.g.,E2E_OVERRIDE_EXTERNAL_ID) and documented as depending on seed data to avoid brittle tests.♻️ Suggested refactor
At the top of the file, after imports:
+# External ID from seeded test data - must match seed configuration +E2E_OVERRIDE_EXTERNAL_ID = "e2e-seeded-override" + pytestmark = [pytest.mark.flaky]Then update line 32:
filtered_billing_overrides = ( mpt_ops.billing.manual_overrides.filter(RQLQuery(id=billing_override_id)) - .filter(RQLQuery(externalId="e2e-seeded-override")) + .filter(RQLQuery(externalId=E2E_OVERRIDE_EXTERNAL_ID)) .select(*select_fields) )tests/e2e/billing/override/test_sync_override.py (2)
9-12: Consider strengthening the assertion.The test only verifies that
result is not None. Consider also asserting thatresult.id == billing_override_idto confirm the correct resource was retrieved.💡 Suggested enhancement
def test_get_billing_override_by_id(mpt_ops, billing_override_id): result = mpt_ops.billing.manual_overrides.get(billing_override_id) assert result is not None + assert result.id == billing_override_id
28-38: Extract hardcoded externalId to a named constant.Line 32 uses the hardcoded string
"e2e-seeded-override". Based on learnings, hardcoded external IDs in e2e tests are intentional due to seeded test data, but should be extracted to a named constant (e.g.,E2E_OVERRIDE_EXTERNAL_ID) and documented as depending on seed data to avoid brittle tests.♻️ Suggested refactor
At the top of the file, after imports:
+# External ID from seeded test data - must match seed configuration +E2E_OVERRIDE_EXTERNAL_ID = "e2e-seeded-override" + pytestmark = [pytest.mark.flaky]Then update line 32:
filtered_billing_overrides = ( mpt_ops.billing.manual_overrides.filter(RQLQuery(id=billing_override_id)) - .filter(RQLQuery(externalId="e2e-seeded-override")) + .filter(RQLQuery(externalId=E2E_OVERRIDE_EXTERNAL_ID)) .select(*select_fields) )
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e_config.test.jsontests/e2e/billing/override/conftest.pytests/e2e/billing/override/test_async_override.pytests/e2e/billing/override/test_sync_override.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/override/test_async_override.pye2e_config.test.jsontests/e2e/billing/override/test_sync_override.pytests/e2e/billing/override/conftest.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/override/test_async_override.pytests/e2e/billing/override/test_sync_override.pytests/e2e/billing/override/conftest.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/override/test_async_override.pytests/e2e/billing/override/test_sync_override.pytests/e2e/billing/override/conftest.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/override/test_async_override.pytests/e2e/billing/override/test_sync_override.pytests/e2e/billing/override/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/billing/override/test_async_override.pytests/e2e/billing/override/test_sync_override.pytests/e2e/billing/override/conftest.py
🧬 Code graph analysis (3)
tests/e2e/billing/override/test_async_override.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)
async_mpt_ops(36-39)tests/e2e/billing/override/conftest.py (2)
billing_override_id(5-6)invalid_billing_override_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/billing/override/test_sync_override.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_ops(31-32)tests/e2e/billing/override/conftest.py (2)
billing_override_id(5-6)invalid_billing_override_id(10-11)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/billing/override/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
⏰ 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 (4)
e2e_config.test.json (1)
24-24: LGTM!The billing override ID configuration follows the established pattern and correctly supports the new test fixtures.
tests/e2e/billing/override/conftest.py (1)
4-11: LGTM!Both fixtures follow established e2e testing patterns:
billing_override_idreuses existing seeded data for read-only operations (speeding up test execution), andinvalid_billing_override_idprovides a deterministic invalid value for negative testing.Based on learnings, reusing existing API resources via fixtures like these is the preferred approach for e2e tests.
tests/e2e/billing/override/test_async_override.py (1)
6-6: Verify the necessity of marking all tests as flaky.All tests in this module are marked with
pytest.mark.flaky. Per coding guidelines, this marker should indicate intermittent stability concerns. Are these tests known to be unstable, or is this marker preemptive? If instability is expected, please document the root cause.Consider investigating and resolving the underlying instability rather than marking tests flaky, or remove the marker if tests prove stable.
tests/e2e/billing/override/test_sync_override.py (1)
6-6: Verify the necessity of marking all tests as flaky.All tests in this module are marked with
pytest.mark.flaky. Per coding guidelines, this marker should indicate intermittent stability concerns. Are these tests known to be unstable, or is this marker preemptive? If instability is expected, please document the root cause.Consider investigating and resolving the underlying instability rather than marking tests flaky, or remove the marker if tests prove stable.
robcsegal
force-pushed
the
MPT-14926-add-e-2-e-tests-for-billing-overrides
branch
from
02f32fc to
1ec8ae9
Compare
|
Added e2e tests for billing overrides
https://softwareone.atlassian.net/browse/MPT-14926
Closes MPT-14926
billing.override.id = "BOV-7202-7714"ine2e_config.test.jsontests/e2e/billing/override/conftest.py:billing_override_id(e2e_config)(readsbilling.override.id)invalid_billing_override_id()returns"BOV-0000-0000"tests/e2e/billing/override/test_async_override.py:tests/e2e/billing/override/test_sync_override.py: