refactor(integration-tests): Deprecate clp_mode_utils in favour of per-package mode definitions.#2243
Conversation
WalkthroughThis PR refactors the test infrastructure to simplify validation logic. Mode configurations are consolidated into dedicated utility modules, the validation function is renamed and streamlined to check only running Docker services (removing mode config correctness checks), and unused utility functions are removed. ChangesTest Infrastructure Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
clp_mode_utils module.clp_mode_utils and adopt package-specific mode utilities; Remove problematic running-mode validation for clp-presto and clp-json-s3 tests.
clp_mode_utils and adopt package-specific mode utilities; Remove problematic running-mode validation for clp-presto and clp-json-s3 tests.clp_mode_utils and adopt package-specific mode utilities; remove problematic running-mode validation for clp-presto and clp-json-s3 tests.
junhaoliao
left a comment
There was a problem hiding this comment.
posted some questions. the code lgtm
for the title, how about:
refactor(integration-tests): Deprecate `clp_mode_utils` in favour of per-package mode definitions.
| logger.info("Starting test: 'test_clp_json_startup'") | ||
|
|
||
| validate_package_instance(fixt_package_instance) | ||
| validate_package_running(fixt_package_instance) |
There was a problem hiding this comment.
qq: is there a reason not to put validate_package_running() calls into the fixture. it appears that everywhere the fixture is used we have been calling this validate_package_running(). is it to avoid coupling of some sort?
| verify_package_compression, | ||
| ) | ||
| from tests.utils.clp_mode_utils import CLP_API_SERVER_COMPONENT, CLP_BASE_COMPONENTS | ||
| from tests.utils.config import PackageCompressionJob, PackageInstance, PackageModeConfig |
There was a problem hiding this comment.
just to confirm my understanding, we will eventually phase out PackageModeConfig and replace it with ClpPackageModeConfig right? the PackageModeConfig removal is to be submitted later?
There was a problem hiding this comment.
That's correct; I'll work that removal into #2231.
clp_mode_utils and adopt package-specific mode utilities; remove problematic running-mode validation for clp-presto and clp-json-s3 tests.clp_mode_utils in favour of per-package mode definitions.
Description
This PR deprecates the
clp_mode_utilsmodule in favour of the mode descriptions fromtests/package_tests/clp_*/utils/mode.py.Checklist
breaking change.
Validation performed
Ran
uv run pytest -m 'package'anduv run pytest -m 'core'; all tests pass.Summary by CodeRabbit
Tests
Chores