Conversation
📝 WalkthroughWalkthroughThe changes introduce support for parameter groups under programs by adding a new ParameterGroup model, ParameterGroupsService and AsyncParameterGroupsService classes, a parameter_groups() factory method to ProgramsService, test fixtures, and both unit and end-to-end test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
robcsegal
force-pushed
the
MPT-20326-add-endpoints-and-e-2-e-tests-for-program-parameter-groups
branch
from
2527119 to
78664e1
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: 1
🧹 Nitpick comments (2)
tests/e2e/program/program/parameter_group/test_async_parameter_group.py (1)
43-49: Async delete test has the same weak-verification pattern as sync.Consider validating deletion explicitly (e.g., follow-up
get()failure) and avoid reusing a teardown-deleting fixture for this test to prevent expected teardown noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/parameter_group/test_async_parameter_group.py` around lines 43 - 49, The test_delete_parameter_group currently calls delete on a fixture-created resource and lacks an explicit verification; modify test_delete_parameter_group to create its own parameter group (do not use the teardown-deleting fixture created_parameter_group), call await async_mpt_vendor.program.programs.parameter_groups(program_id).delete(created.id), then attempt a follow-up await async_mpt_vendor.program.programs.parameter_groups(program_id).get(created.id) and assert the expected failure (e.g., raise/HTTP 404 or falsey response) to prove deletion; ensure you reference the parameter_groups(...) helper and the delete/get methods when implementing the create->delete->verify flow.tests/e2e/program/program/parameter_group/test_sync_parameter_group.py (1)
43-49: Strengthen the delete test assertion and avoid the guaranteed double-delete path.
test_delete_parameter_groupperforms delete with no verification, and teardown then tries to delete the same resource again. Consider creating a resource scoped to this test and asserting a subsequentget()fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/parameter_group/test_sync_parameter_group.py` around lines 43 - 49, test_delete_parameter_group currently deletes a shared fixture resource without verifying deletion and causes teardown to double-delete; update the test to create a parameter group scoped to this test (call programs.parameter_groups(program_id).create(...) or otherwise create a fresh resource instead of using created_parameter_group), call result.delete(new_parameter_group.id), then assert deletion by calling result.get(new_parameter_group.id) and expecting a failure (e.g., raise HTTP 404 or None) to confirm removal; this avoids using the shared created_parameter_group fixture and prevents the guaranteed double-delete path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/program/program/parameter_group/test_sync_parameter_group.py`:
- Line 17: Ruff is reporting RUF102 for the intentional "noqa: WPS421"
suppression on the print line "TEARDOWN - Unable to delete parameter group …",
so update the Ruff configuration to recognize wemake-style external rule codes
(WPS) so Ruff respects the WPS421 noqa; specifically, add WPS to the
external-rule/external codes in your Ruff config (pyproject.toml/.ruff.toml) or
otherwise map/allow WPS codes so the "noqa: WPS421" on that teardown print
statement is honored and RUF102 is no longer emitted.
---
Nitpick comments:
In `@tests/e2e/program/program/parameter_group/test_async_parameter_group.py`:
- Around line 43-49: The test_delete_parameter_group currently calls delete on a
fixture-created resource and lacks an explicit verification; modify
test_delete_parameter_group to create its own parameter group (do not use the
teardown-deleting fixture created_parameter_group), call await
async_mpt_vendor.program.programs.parameter_groups(program_id).delete(created.id),
then attempt a follow-up await
async_mpt_vendor.program.programs.parameter_groups(program_id).get(created.id)
and assert the expected failure (e.g., raise/HTTP 404 or falsey response) to
prove deletion; ensure you reference the parameter_groups(...) helper and the
delete/get methods when implementing the create->delete->verify flow.
In `@tests/e2e/program/program/parameter_group/test_sync_parameter_group.py`:
- Around line 43-49: test_delete_parameter_group currently deletes a shared
fixture resource without verifying deletion and causes teardown to
double-delete; update the test to create a parameter group scoped to this test
(call programs.parameter_groups(program_id).create(...) or otherwise create a
fresh resource instead of using created_parameter_group), call
result.delete(new_parameter_group.id), then assert deletion by calling
result.get(new_parameter_group.id) and expecting a failure (e.g., raise HTTP 404
or None) to confirm removal; this avoids using the shared
created_parameter_group fixture and prevents the guaranteed double-delete path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d48b2f25-fe2a-4cd5-98f1-f83b3359e612
📒 Files selected for processing (8)
e2e_config.test.jsonmpt_api_client/resources/program/programs.pympt_api_client/resources/program/programs_parameter_groups.pytests/e2e/program/program/parameter_group/conftest.pytests/e2e/program/program/parameter_group/test_async_parameter_group.pytests/e2e/program/program/parameter_group/test_sync_parameter_group.pytests/unit/resources/program/test_programs.pytests/unit/resources/program/test_programs_parameter_groups.py
robcsegal
deleted the
MPT-20326-add-endpoints-and-e-2-e-tests-for-program-parameter-groups
branch
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
2 participants
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 managing program parameter groups in the API client. It introduces new service classes for both synchronous and asynchronous operations on parameter groups, updates the main
Programservices to expose these new capabilities, and provides comprehensive unit and end-to-end tests to verify the new functionality.New Program Parameter Groups Support:
ParameterGroupmodel andParameterGroupsService/AsyncParameterGroupsServiceclasses toprograms_parameter_groups.py, including endpoint configuration and resource mixins.ProgramandAsyncProgramsServiceclasses inprograms.pyto provide.parameter_groups(program_id)methods for accessing the new services. [1] [2] [3]Testing Enhancements:
test_programs_parameter_groups.py, covering endpoints, method presence, and model field parsing.test_sync_parameter_group.pyandtest_async_parameter_group.py, with supporting fixtures inconftest.py. [1] [2] [3]Programservices to check for the presence of the new.parameter_groupsmethods. [1] [2] [3]Configuration Updates:
e2e_config.test.jsonfor use in tests.Closes MPT-20326
ParameterGroupmodel with typed fields for metadata (name, label, description, display_order, default), counts (parameter_count), and relationships (program, audit)ParameterGroupsServiceandAsyncParameterGroupsServicewith endpoint configuration at/public/v1/program/programs/{program_id}/parameter-groupsand support for managed resource operations (create, read, update, delete, list, filter, select)ProgramsServiceandAsyncProgramsServicewithparameter_groups(program_id)factory methods for accessing parameter group services