MPT-14898 E2E for catalog units of measure by albertsola · Pull Request #146 · softwareone-platform/mpt-api-python-client

albertsola · 2025-12-03T16:06:20Z

Summary by CodeRabbit

Tests
- Added comprehensive end-to-end test coverage for units-of-measure in the catalog: synchronous and asynchronous tests for retrieval, filtering, update, create (placeholder/skipped) and not-found error handling; module-level flaky markers applied.
- Added pytest fixtures to expose the units-of-measure service and a configured unit ID for tests.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-03T16:06:31Z

Walkthrough

Adds end-to-end pytest fixtures and parallel async/sync test modules for the catalog units_of_measure service, covering retrieval, filtering, update, and not-found error cases; creation tests are present but skipped.

Changes

Cohort / File(s)	Summary
Conftest / fixtures `tests/e2e/catalog/units_of_measure/conftest.py`	Adds two pytest fixtures: `units_of_measure_service` (returns `mpt_ops.catalog.units_of_measure`) and `unit_of_measure_id` (returns `e2e_config["catalog.unit.id"]`).
Async e2e tests `tests/e2e/catalog/units_of_measure/test_async_units_of_measure.py`	Adds async tests and fixture: retrieval by ID, filtering with iterate+sort, 404 not-found assertion, update verification, and a skipped create placeholder; module marked flaky.
Sync e2e tests `tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py`	Adds sync tests mirroring async: retrieval, filtering, 404 not-found assertion, update verification, and a skipped create placeholder; module marked flaky.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Tests are repetitive across async/sync, lowering per-file novelty but require checking correctness of helpers and error assertions.
Pay attention to:
- Fixture wiring (mpt_ops vs async_mpt_ops) and returned service objects.
- Use of unit_of_measure_id config value in filter/update tests.
- Error assertion expecting MPTAPIError and exact message matching.
- Skipped create tests — ensure skip markers are intentional.

Suggested reviewers

d3rky
jentyk
svazquezco
alephsur
ruben-sebrango

Poem

🐇 I hopped into tests, both async and clear,

fetching a unit, then updating with cheer.
Filters and errors, each case I did meet,
fixtures snug in their conftest seat.
A tiny rabbit clap — these tests are neat! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the main change: adding end-to-end tests for catalog units of measure functionality.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch e2e/catalog/MPT-14898-units-of-measure

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9f9af6e and 81f02f9.

📒 Files selected for processing (3)

tests/e2e/catalog/units_of_measure/conftest.py (1 hunks)
tests/e2e/catalog/units_of_measure/test_async_units_of_measure.py (1 hunks)
tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

tests/e2e/catalog/units_of_measure/test_async_units_of_measure.py
tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py

🧰 Additional context used

🧠 Learnings (2)

📓 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-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/catalog/units_of_measure/conftest.py

🧬 Code graph analysis (1)

tests/e2e/catalog/units_of_measure/conftest.py (1)

tests/e2e/conftest.py (2)

mpt_ops (31-32)

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 (2)

tests/e2e/catalog/units_of_measure/conftest.py (2)

1-7: Service fixture is simple and matches existing e2e patterns

units_of_measure_service cleanly exposes mpt_ops.catalog.units_of_measure via a dedicated fixture and should integrate well with both sync and async tests that build on top of it. No issues spotted.

9-11: Config‑backed ID fixture is appropriate for E2E

unit_of_measure_id correctly pulls the unit ID from e2e_config["catalog.unit.id"], giving a single source of truth for tests; failing fast with KeyError if misconfigured is reasonable for E2E. Looks good as‑is.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-12-03T16:06:43Z

✅ Found Jira issue key in the title: MPT-14898

Generated by 🚫 dangerJS against 81f02f9

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3654390 and 9f9af6e.

📒 Files selected for processing (3)

tests/e2e/catalog/units_of_measure/conftest.py (1 hunks)
tests/e2e/catalog/units_of_measure/test_async_units_of_measure.py (1 hunks)
tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

tests/e2e/catalog/units_of_measure/conftest.py (1)

tests/e2e/conftest.py (2)

mpt_ops (31-32)

e2e_config (89-92)

tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py (6)

mpt_api_client/exceptions.py (1)

MPTAPIError (20-42)

tests/e2e/helper.py (1)

assert_service_filter_with_iterate (42-50)

tests/e2e/catalog/units_of_measure/test_async_units_of_measure.py (1)

test_create (15-19)

tests/e2e/catalog/units_of_measure/conftest.py (2)

units_of_measure_service (5-6)

unit_of_measure_id (10-11)

tests/e2e/conftest.py (1)

short_uuid (101-102)

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 (11)

tests/e2e/catalog/units_of_measure/conftest.py (1)

4-11: LGTM!

The fixtures are well-defined and follow pytest conventions. They provide clean access to the units of measure service and test configuration.

tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py (4)

9-14: LGTM!

The test is appropriately marked as skipped with a clear reason. The placeholder implementation looks correct for when the create functionality is ready.

17-20: LGTM!

The test correctly verifies that the retrieved unit of measure has the expected ID.

31-35: LGTM!

The test correctly verifies that a 404 error is raised for non-existent IDs.

38-45: LGTM!

The test correctly updates a unit of measure and verifies the change. Note that this test mutates shared test data, which aligns with the module's flaky marker indicating awareness of potential test isolation concerns.

tests/e2e/catalog/units_of_measure/test_async_units_of_measure.py (6)

9-11: LGTM!

The async service fixture is correctly defined using async_mpt_ops. Defining it locally in the test file is appropriate for async-specific fixtures.

14-19: LGTM!

The async test placeholder is correctly marked as skipped and uses proper async/await syntax.

22-25: LGTM!

The async test correctly retrieves a unit of measure and verifies the ID matches. Proper use of await.

28-31: LGTM!

The async filter test correctly uses the unit_of_measure_id fixture and properly awaits the helper function.

34-38: LGTM!

The async test correctly verifies 404 error handling with proper use of pytest.raises and await.

41-50: LGTM!

The async update test correctly uses await and verifies the name change. Like the sync version, this mutates shared data, which is acknowledged by the flaky marker.

tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py

sonarqubecloud · 2025-12-03T16:38:25Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

albertsola requested review from alephsur, d3rky, jentyk, robcsegal, ruben-sebrango and svazquezco as code owners December 3, 2025 16:06

coderabbitai bot reviewed Dec 3, 2025

View reviewed changes

tests/e2e/catalog/units_of_measure/test_sync_units_of_measure.py Outdated Show resolved Hide resolved

MPT-14898 E2E for catalog units of measure

81f02f9

albertsola force-pushed the e2e/catalog/MPT-14898-units-of-measure branch from 9f9af6e to 81f02f9 Compare December 3, 2025 16:22

robcsegal approved these changes Dec 3, 2025

View reviewed changes

svazquezco approved these changes Dec 3, 2025

View reviewed changes

albertsola merged commit 7d33e5d into main Dec 3, 2025
6 checks passed

albertsola deleted the e2e/catalog/MPT-14898-units-of-measure branch December 3, 2025 16:44

coderabbitai bot mentioned this pull request Dec 8, 2025

MPT-16242 Add iterate in expected methods for catalog resources #150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPT-14898 E2E for catalog units of measure#146

MPT-14898 E2E for catalog units of measure#146
albertsola merged 1 commit intomainfrom
e2e/catalog/MPT-14898-units-of-measure

albertsola commented Dec 3, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 3, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 3, 2025 •

edited by atlassian bot

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

albertsola commented Dec 3, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Dec 3, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 3, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

albertsola commented Dec 3, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 3, 2025 •

edited

Loading

github-actions bot commented Dec 3, 2025 •

edited by atlassian bot

Loading