feat(integration-tests): Validate that the components of the CLP package are running (fixes #1644).#1659
Conversation
…ig file; address comments.
…er-level clp-config.yml
…E_CONFIGS other than clp-text and clp-json
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
integration-tests/tests/test_package_start.py(2 hunks)integration-tests/tests/utils/asserting_utils.py(2 hunks)integration-tests/tests/utils/config.py(4 hunks)integration-tests/tests/utils/docker_utils.py(1 hunks)integration-tests/tests/utils/utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/package-template/src/sbin/del-archives.sh:6-9
Timestamp: 2024-11-15T16:28:08.644Z
Learning: For wrapper scripts in the `components/package-template/src/sbin/` directory, keep them simple and avoid adding additional validation code.
📚 Learning: 2025-07-28T08:33:57.487Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:133-137
Timestamp: 2025-07-28T08:33:57.487Z
Learning: In CLP integration tests, the `run_and_assert` utility function should only be used for commands that test CLP package functionality (like clp/clp-s compression/decompression operations). For processing or helper commands (like jq, sort, etc.), direct `subprocess.run` calls should be used instead, as they serve different purposes and don't need the same error handling patterns.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-08-17T16:10:38.722Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-16T10:24:29.316Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/pyproject.toml:11-13
Timestamp: 2025-08-16T10:24:29.316Z
Learning: In the CLP project's integration-tests directory, the integration tests package is not intended to be shipped or distributed - only the tests are leveraged for local testing purposes, so console script entry points should not be included in pyproject.toml.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-10-07T07:54:32.427Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.
Applied to files:
integration-tests/tests/utils/config.py
🧬 Code graph analysis (2)
integration-tests/tests/test_package_start.py (2)
integration-tests/tests/utils/asserting_utils.py (1)
validate_package_running(37-67)integration-tests/tests/fixtures/package_instance.py (1)
fixt_package_instance(18-34)
integration-tests/tests/utils/config.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
ClpConfig(705-955)
⏰ 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: package-image
🔇 Additional comments (11)
integration-tests/tests/utils/docker_utils.py (1)
7-18: LGTM!The function correctly uses
shutil.whichto locate the Docker binary and raises an appropriateRuntimeErrorif not found.integration-tests/tests/utils/config.py (4)
5-13: LGTM!The new imports for
reand CLP configuration utilities are appropriate for the added functionality.
78-79: LGTM!The
clp_log_dirfield is correctly initialized using the CLP default log directory path constant, and the directory is created if it doesn't exist.Also applies to: 100-109
166-179: LGTM!The
clp_instance_idfield initialization correctly reads from the instance-id file and validates it using the helper method.
181-203: LGTM!The
_get_clp_instance_idmethod properly validates the instance ID format as a 4-character hexadecimal string with clear error messages for both read failures and format validation failures.integration-tests/tests/test_package_start.py (2)
7-9: LGTM!The import change correctly brings in the new validation utility.
18-27: LGTM!The test is now cleaner and delegates validation logic to the dedicated utility function. This improves maintainability and follows good separation of concerns.
integration-tests/tests/utils/utils.py (3)
4-4: LGTM!The
reimport is needed for the newstrip_regex_suffixfunction.
79-91: LGTM!The
strip_regex_suffixfunction correctly handles regex-based suffix stripping by anchoring the pattern at the end of the string and returning the original text if no match is found.
66-76: Consider usingstr.removeprefix()if Python 3.9+ is guaranteed.Python 3.9+ provides the built-in
str.removeprefix()method which does exactly this. Verify the project's minimum Python version requirement; if it's 3.9 or higher, you could simplify this function to a one-liner or directly use the built-in in callers.integration-tests/tests/utils/asserting_utils.py (1)
10-12: LGTM!The imports correctly bring in all necessary utilities for the validation function.
validate_package_running.There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
integration-tests/tests/utils/asserting_utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
🧬 Code graph analysis (1)
integration-tests/tests/utils/asserting_utils.py (3)
integration-tests/tests/utils/config.py (1)
PackageInstance(160-203)integration-tests/tests/utils/docker_utils.py (1)
list_running_containers_in_compose_project(21-48)integration-tests/tests/utils/utils.py (1)
strip_prefix(66-76)
⏰ 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). (5)
- GitHub Check: package-image
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
integration-tests/tests/utils/asserting_utils.py (1)
10-12: Imports correctly reflect new helpers and typesThe new imports line up with the utilities and config objects used below; no issues here.
| logger.info("The '%s' package has been spun up successfully.", mode_name) | ||
|
|
||
| component_list = fixt_package_instance.package_config.component_list | ||
| logger.info( |
There was a problem hiding this comment.
remove this redundant variable (outside line diff range).
There was a problem hiding this comment.
which variable?
| logger.info("The '%s' package has been spun up successfully.", mode_name) | ||
|
|
||
| component_list = fixt_package_instance.package_config.component_list | ||
| logger.info( |
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @Bill-hbrhbr 's review
| docker_bin = get_binary_path("docker") | ||
|
|
||
| # fmt: off | ||
| compose_ps_cmd = [ | ||
| docker_bin, |
There was a problem hiding this comment.
can we simply write
| docker_bin = get_binary_path("docker") | |
| # fmt: off | |
| compose_ps_cmd = [ | |
| docker_bin, | |
| # fmt: off | |
| compose_ps_cmd = [ | |
| "/usr/bin/env", | |
| "docker" |
if the docker_bin absolute path isn't consumed (e.g. logged) anywhere?
There was a problem hiding this comment.
it doesn't really seem like there's a clear benefit to this change; plus, using get_binary_path gives a clear error message when Docker is not on PATH, so eliminating it would remove that. @junhaoliao could you elaborate?
There was a problem hiding this comment.
ah that was more of a question, cuz i don't know the background of adding get_binary_path()
i don't really have a preference. practically, i feel they do the same thing - look up the environment to find an executable in PATH and run it.
if you ask me to list some benefits for the approaches -
For env:
envresolves the binary at exec time, not before.- With
env, we get an/usr/bin/env: ‘docker’: No such file or directoryOS error and don't need to manually check the return value ofshutil.which(). in a sense, we don't reinvent the wheels
For get_binary_path():
- we can have the error wrapped in a Python exception and customize the error message
are there things i missed? to be clear, i'm fine keeping get_binary_path if we want an explicit Python RuntimeException
There was a problem hiding this comment.
resolved offline
There was a problem hiding this comment.
a summary for record keeping:
get_binary_pathwas added in an effort to "suppress" ruff S607. it is considered a suppression because we're still getting the binary from the exec environment.- Using binaries from the environment is fine in the test code (, until the day we're targeted by some supply chain attacks)
- The
get_binary_pathwas decided to be kept for this specific reason: we can have the error wrapped in a Python exception and customize the error message
802f743
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @Bill-hbrhbr 's review
| docker_bin = get_binary_path("docker") | ||
|
|
||
| # fmt: off | ||
| compose_ps_cmd = [ | ||
| docker_bin, |
There was a problem hiding this comment.
a summary for record keeping:
get_binary_pathwas added in an effort to "suppress" ruff S607. it is considered a suppression because we're still getting the binary from the exec environment.- Using binaries from the environment is fine in the test code (, until the day we're targeted by some supply chain attacks)
- The
get_binary_pathwas decided to be kept for this specific reason: we can have the error wrapped in a Python exception and customize the error message
…age are running (fixes y-scope#1644). (y-scope#1659) Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
…age are running (fixes y-scope#1644). (y-scope#1659) Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
…age are running (fixes y-scope#1644). (y-scope#1659) Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
Description
This PR introduces the
validate_package_runningfunction to check if the containerized components of the given package are all running after spin-up.validate_package_runninguseslist_running_containers_in_compose_projectto get the list of running components, and processes this list in such a way that a 1:1 comparison can be performed against therequired_componentslist for that package.PR dependencies
This PR depends on the following being merged:
PackageConfigbased on the CLP package config. #1658Checklist
breaking change.
Validation performed
Ran the following command from
clp/integration-tests:uv run pytest -m 'package'Both the
clp-jsonandclp-texttests passed.Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.