feat(integration-tests): Test the startup and teardown of the clp-text and clp-json packages.
#1437
Conversation
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
Contributor
WalkthroughAdds integration-test infrastructure: new pytest marker/flags, fixtures to create/start/stop CLP package instances, utilities for config, Docker inspection and package lifecycle, a parameterized startup test, and a task to run package-focused integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant clp_package as Fixture:clp_package
participant clp_config as Fixture:clp_config
participant PackageUtils as package_utils
participant DockerUtils as docker_utils
Test->>clp_package: request fixture
clp_package->>clp_config: request config
clp_config->>clp_config: build temp YAML config
clp_config-->>clp_package: return PackageConfig
clp_package->>PackageUtils: start_clp_package(config)
PackageUtils->>PackageUtils: run start script (subprocess)
clp_package->>clp_package: create PackageInstance (reads instance id)
clp_package-->>Test: yield PackageInstance
Test->>PackageUtils: is_package_running(instance)
PackageUtils->>DockerUtils: list_prefixed_containers(...)
PackageUtils->>DockerUtils: inspect_container_state(...)
PackageUtils-->>Test: (bool, message)
Test->>PackageUtils: is_running_mode_correct(instance)
PackageUtils->>PackageUtils: load shared config, validate, compute signatures
PackageUtils-->>Test: (bool, message)
Test->>clp_package: teardown
clp_package->>PackageUtils: stop_clp_package(instance)
PackageUtils->>PackageUtils: run stop script (subprocess)
clp_package->>clp_config: cleanup temp files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (12)📓 Common learnings📚 Learning: 2025-08-16T10:24:29.316ZApplied to files:
📚 Learning: 2025-08-17T16:10:38.722ZApplied to files:
📚 Learning: 2025-09-26T21:12:21.655ZApplied to files:
📚 Learning: 2025-07-03T13:33:27.460ZApplied to files:
📚 Learning: 2025-07-28T08:33:57.487ZApplied to files:
📚 Learning: 2025-08-09T04:07:27.083ZApplied to files:
📚 Learning: 2025-08-20T22:07:04.953ZApplied to files:
📚 Learning: 2025-05-06T09:48:55.408ZApplied to files:
📚 Learning: 2025-05-06T10:07:04.654ZApplied to files:
📚 Learning: 2025-07-07T17:41:15.655ZApplied to files:
📚 Learning: 2025-09-02T15:23:42.507ZApplied to files:
🧬 Code graph analysis (1)integration-tests/tests/utils/config.py (3)
⏰ 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). (13)
🔇 Additional comments (7)
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 |
Bill-hbrhbr
requested changes
Oct 21, 2025
Contributor
There was a problem hiding this comment.
Looks pretty good for a first draft. Still, I feel like quite a bit of refactoring will be needed, and I will def take a closer look at everything after the testing infra migrates a bit.
Some points of importance:
- There's definitely no need to modify an existing
clp_config.yamlin place. We could keep clp config JSON objects in memory, and dump them into either temp files or log files when we pass them to the package start up script. - For clp config JSON object, we can see if we can maximize the amount of stuff we can reuse in (clp_config.py)[https://github.com/y-scope/clp/blob/94b501b264ab376e2c917fa47b61dd91fca50132/components/clp-py-utils/clp_py_utils/clp_config.py#L28]
- See which fixture scope the package instance should assume. You can find the list here. Personally, I believe the
module(per-file) scope is the best. - We need new taskfile tasks to automate the new package tests. But we can address that when doing final touch ups.
- Misc discussion points that you can find in my individual comments. Let me know if you disagree with whatever I've posted or have better solutions.
…ig file; address comments.
…er-level clp-config.yml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
integration-tests/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
integration-tests/.pytest.ini(2 hunks)integration-tests/pyproject.toml(1 hunks)integration-tests/tests/conftest.py(1 hunks)integration-tests/tests/fixtures/integration_test_config.py(1 hunks)integration-tests/tests/fixtures/package_config_fixtures.py(1 hunks)integration-tests/tests/fixtures/package_instance_fixtures.py(1 hunks)integration-tests/tests/test_package_start.py(1 hunks)integration-tests/tests/utils/config.py(3 hunks)integration-tests/tests/utils/docker_utils.py(1 hunks)integration-tests/tests/utils/package_utils.py(1 hunks)integration-tests/tests/utils/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 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: 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.
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: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
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.
📚 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/pyproject.tomlintegration-tests/.pytest.iniintegration-tests/tests/test_package_start.pyintegration-tests/tests/conftest.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/fixtures/integration_test_config.pyintegration-tests/tests/fixtures/package_config_fixtures.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/fixtures/package_instance_fixtures.pyintegration-tests/.pytest.iniintegration-tests/tests/test_package_start.pyintegration-tests/tests/conftest.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/fixtures/integration_test_config.pyintegration-tests/tests/fixtures/package_config_fixtures.py
📚 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/.pytest.iniintegration-tests/tests/test_package_start.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/config.py
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-10-18T15:38:01.210Z
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1417
File: components/clp-py-utils/clp_py_utils/serialization_utils.py:6-13
Timestamp: 2025-10-18T15:38:01.210Z
Learning: In the y-scope/clp repository, parameter descriptions in docstrings are intentionally omitted when the parameter name is self-explanatory. This is the project's preferred documentation style.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T22:07:04.953Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:121-123
Timestamp: 2025-08-20T22:07:04.953Z
Learning: For the CLP integration tests codebase, do not suggest deriving tarball filenames from URLs instead of hard-coding ".tar.gz" extension. The user has explicitly rejected this suggestion.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T15:23:42.507Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:59-62
Timestamp: 2025-09-02T15:23:42.507Z
Learning: In the CoreConfig class (integration-tests/tests/utils/config.py), clp_binary_path and clp_s_binary_path are decorated with property, so they should be accessed as attributes (without parentheses) rather than method calls.
Applied to files:
integration-tests/tests/utils/config.py
🧬 Code graph analysis (7)
integration-tests/tests/fixtures/package_instance_fixtures.py (3)
integration-tests/tests/utils/config.py (2)
PackageInstance(137-186)PackageInstanceConfig(123-133)integration-tests/tests/utils/package_utils.py (2)
start_clp_package(157-171)stop_clp_package(174-189)integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(53-73)
integration-tests/tests/test_package_start.py (3)
integration-tests/tests/utils/config.py (1)
PackageInstance(137-186)integration-tests/tests/utils/package_utils.py (2)
is_package_running(192-225)is_running_mode_correct(228-243)integration-tests/tests/fixtures/package_instance_fixtures.py (1)
clp_package(21-48)
integration-tests/tests/conftest.py (1)
integration-tests/tests/utils/utils.py (1)
get_env_var(11-21)
integration-tests/tests/utils/package_utils.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(53-73)components/clp-py-utils/clp_py_utils/clp_config.py (3)
Package(135-159)QueryEngine(111-114)StorageEngine(95-97)integration-tests/tests/utils/config.py (5)
PackageInstance(137-186)PackageInstanceConfig(123-133)PackageModeConfig(114-119)start_script_path(103-105)stop_script_path(108-110)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)
integration-tests/tests/utils/config.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(53-73)components/clp-py-utils/clp_py_utils/clp_config.py (2)
QueryEngine(111-114)StorageEngine(95-97)integration-tests/tests/utils/utils.py (3)
unlink(54-75)validate_dir_exists(78-85)validate_file_exists(88-95)integration-tests/tests/fixtures/integration_test_config.py (1)
package_config(24-30)
integration-tests/tests/fixtures/integration_test_config.py (1)
integration-tests/tests/utils/utils.py (1)
get_env_var(11-21)
integration-tests/tests/fixtures/package_config_fixtures.py (3)
integration-tests/tests/utils/config.py (3)
PackageConfig(64-110)PackageInstanceConfig(123-133)PackageModeConfig(114-119)integration-tests/tests/utils/package_utils.py (2)
get_dict_from_mode_name(69-76)write_temp_config_file(107-129)integration-tests/tests/fixtures/integration_test_config.py (1)
package_config(24-30)
🔇 Additional comments (7)
integration-tests/.pytest.ini (1)
6-6: LGTM!The
-rAflag provides comprehensive test reporting, and the newpackagemarker properly categorizes the package lifecycle tests.Also applies to: 22-22
integration-tests/tests/utils/utils.py (1)
88-95: LGTM!The function correctly mirrors
validate_dir_existslogic and provides appropriate file validation with a clear error message.integration-tests/tests/fixtures/integration_test_config.py (1)
27-29: LGTM!The addition of
test_root_dirparameter follows the same pattern used in theintegration_test_configfixture and properly resolves the path.integration-tests/tests/test_package_start.py (1)
19-21: LGTM!The test structure properly uses pytest markers and parametrization to validate package startup across all configured modes.
integration-tests/tests/utils/docker_utils.py (1)
34-37: Clear failure messaging for docker enumeration.Falling back to stderr/stdout before raising keeps the runtime diagnostics actionable, especially when docker itself returns terse messages.
integration-tests/tests/utils/config.py (1)
170-185: Great instance-id validation.The strict 4-digit hex check will catch corrupt logs early and gives a precise failure path back to the operator.
integration-tests/tests/utils/package_utils.py (1)
124-127: Appreciate the atomic config write.Dropping the YAML to a tmp file and replacing avoids the half-written config issue we’ve run into before.
…E_CONFIGS other than clp-text and clp-json
Contributor
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 (2)
integration-tests/tests/utils/config.py(3 hunks)integration-tests/tests/utils/package_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 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: 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.
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: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
📚 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/package_utils.pyintegration-tests/tests/utils/config.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/package_utils.pyintegration-tests/tests/utils/config.py
📚 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/utils/package_utils.pyintegration-tests/tests/utils/config.py
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T22:07:04.953Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:121-123
Timestamp: 2025-08-20T22:07:04.953Z
Learning: For the CLP integration tests codebase, do not suggest deriving tarball filenames from URLs instead of hard-coding ".tar.gz" extension. The user has explicitly rejected this suggestion.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T15:23:42.507Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:59-62
Timestamp: 2025-09-02T15:23:42.507Z
Learning: In the CoreConfig class (integration-tests/tests/utils/config.py), clp_binary_path and clp_s_binary_path are decorated with property, so they should be accessed as attributes (without parentheses) rather than method calls.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-03T13:33:27.460Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-07-03T13:33:27.460Z
Learning: In the CLP codebase, `from __future__ import annotations` imports are used to enable Python pipe notation for union types (e.g., `int | str`) in Python versions before 3.10, and should not be considered unused imports.
Applied to files:
integration-tests/tests/utils/config.py
🧬 Code graph analysis (2)
integration-tests/tests/utils/package_utils.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(53-73)components/clp-py-utils/clp_py_utils/clp_config.py (4)
CLPConfig(613-853)Package(136-160)QueryEngine(112-115)StorageEngine(96-98)integration-tests/tests/utils/config.py (5)
PackageInstance(138-187)PackageInstanceConfig(124-134)PackageModeConfig(116-120)start_script_path(105-107)stop_script_path(110-112)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)
integration-tests/tests/utils/config.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(53-73)components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(613-853)integration-tests/tests/utils/utils.py (3)
unlink(54-75)validate_dir_exists(78-85)validate_file_exists(88-95)integration-tests/tests/fixtures/integration_test_config.py (1)
package_config(24-30)
⏰ 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
integration-tests/pyproject.toml (1)
31-31: Per-module overrides recommended instead of globalignore_missing_imports.Setting
ignore_missing_imports = trueglobally silences all missing import errors project-wide, weakening strict type checking. Sincetypes-PyYAMLis already in dependencies, onlyclp_py_utils(imported intests/utils/config.pyandtests/utils/package_utils.py) lacks stubs.Apply this diff to preserve strict checking for all modules except those genuinely lacking stubs:
[tool.mypy] strict = true -ignore_missing_imports = true + +[[tool.mypy.overrides]] +module = ["clp_py_utils.*"] +ignore_missing_imports = trueintegration-tests/tests/test_package_start.py (1)
38-41: Consider removing redundant assertion.If
is_package_runningcorrectly implements its contract,fail_msgshould always be non-None whenrunningis False. The assertion on Line 40 is redundant in that case.Apply this diff if the contract is guaranteed:
running, fail_msg = is_package_running(package_instance) if not running: - assert fail_msg is not None pytest.fail(fail_msg)The same applies to Lines 56-59 for
is_running_mode_correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
integration-tests/pyproject.toml(2 hunks)integration-tests/tests/fixtures/integration_test_config.py(0 hunks)integration-tests/tests/fixtures/package_config_fixtures.py(1 hunks)integration-tests/tests/fixtures/package_instance_fixtures.py(1 hunks)integration-tests/tests/test_package_start.py(1 hunks)integration-tests/tests/utils/config.py(3 hunks)integration-tests/tests/utils/package_utils.py(1 hunks)
💤 Files with no reviewable changes (1)
- integration-tests/tests/fixtures/integration_test_config.py
🧰 Additional context used
🧠 Learnings (21)
📓 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: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
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.
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: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
📚 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/pyproject.tomlintegration-tests/tests/fixtures/package_config_fixtures.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/test_package_start.py
📚 Learning: 2025-10-22T21:02:31.113Z
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).
Applied to files:
integration-tests/pyproject.toml
📚 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/pyproject.tomlintegration-tests/tests/fixtures/package_config_fixtures.pyintegration-tests/tests/fixtures/package_instance_fixtures.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/test_package_start.py
📚 Learning: 2025-09-26T21:12:21.655Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1338
File: components/clp-py-utils/clp_py_utils/clp_config.py:753-753
Timestamp: 2025-09-26T21:12:21.655Z
Learning: Python linting rules that catch YODA conditions exist but are not enabled for Python modules in the CLP package due to time constraints. The team prefers to avoid YODA conditions in Python for consistency.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-07-03T13:33:27.460Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-07-03T13:33:27.460Z
Learning: In the CLP codebase, `from __future__ import annotations` imports are used to enable Python pipe notation for union types (e.g., `int | str`) in Python versions before 3.10, and should not be considered unused imports.
Applied to files:
integration-tests/pyproject.tomlintegration-tests/tests/utils/config.py
📚 Learning: 2025-10-27T04:05:01.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1507
File: taskfiles/deps/lock.yaml:66-76
Timestamp: 2025-10-27T04:05:01.560Z
Learning: In the CLP project, the `uv` CLI tool is documented as a prerequisite in docs/src/dev-docs/building-package.md and is not installed via a task dependency like the rust toolchain. UV is expected to be available in the environment rather than being provisioned by the taskfile system.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-08-24T17:10:44.565Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: taskfiles/lint.yaml:780-781
Timestamp: 2025-08-24T17:10:44.565Z
Learning: The CLP repository is gradually migrating from Black to Ruff for Python formatting. Some modules haven't been set up for Ruff yet, so there's intentional mixed usage of Black and Ruff formatters during this transition period. This should not be flagged as an inconsistency in PR reviews.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-08-20T08:36:38.391Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/__init__.py:1-1
Timestamp: 2025-08-20T08:36:38.391Z
Learning: In the CLP project, __init__.py files should include short module-level docstrings describing the package purpose, as required by their ruff linting configuration, rather than being left empty.
Applied to files:
integration-tests/pyproject.tomlintegration-tests/tests/utils/package_utils.py
📚 Learning: 2025-08-29T07:31:24.032Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:41-43
Timestamp: 2025-08-29T07:31:24.032Z
Learning: In CLP's build tool installation scripts, uv version constraints should use lower bound constraints (uv>=0.8) rather than exact version pinning, following the same philosophy used for other build tools to accommodate platform differences while ensuring minimum required functionality.
Applied to files:
integration-tests/pyproject.toml
📚 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/utils/config.pyintegration-tests/tests/test_package_start.py
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T22:07:04.953Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:121-123
Timestamp: 2025-08-20T22:07:04.953Z
Learning: For the CLP integration tests codebase, do not suggest deriving tarball filenames from URLs instead of hard-coding ".tar.gz" extension. The user has explicitly rejected this suggestion.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T15:23:42.507Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:59-62
Timestamp: 2025-09-02T15:23:42.507Z
Learning: In the CoreConfig class (integration-tests/tests/utils/config.py), clp_binary_path and clp_s_binary_path are decorated with property, so they should be accessed as attributes (without parentheses) rather than method calls.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
integration-tests/tests/test_package_start.py
📚 Learning: 2025-10-18T15:38:01.210Z
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1417
File: components/clp-py-utils/clp_py_utils/serialization_utils.py:6-13
Timestamp: 2025-10-18T15:38:01.210Z
Learning: In the y-scope/clp repository, parameter descriptions in docstrings are intentionally omitted when the parameter name is self-explanatory. This is the project's preferred documentation style.
Applied to files:
integration-tests/tests/test_package_start.py
🧬 Code graph analysis (5)
integration-tests/tests/fixtures/package_config_fixtures.py (3)
integration-tests/tests/utils/config.py (1)
PackageConfig(66-128)integration-tests/tests/utils/package_utils.py (2)
get_dict_from_mode_name(72-81)write_temp_config_file(104-126)integration-tests/tests/utils/utils.py (1)
get_env_var(11-21)
integration-tests/tests/fixtures/package_instance_fixtures.py (3)
integration-tests/tests/utils/config.py (2)
PackageConfig(66-128)PackageInstance(132-181)integration-tests/tests/utils/package_utils.py (2)
start_clp_package(147-161)stop_clp_package(164-177)integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(51-70)
integration-tests/tests/utils/package_utils.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(51-70)components/clp-py-utils/clp_py_utils/clp_config.py (4)
CLPConfig(613-853)Package(136-160)QueryEngine(112-115)StorageEngine(96-98)integration-tests/tests/utils/config.py (4)
PackageConfig(66-128)PackageInstance(132-181)start_script_path(121-123)stop_script_path(126-128)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)
integration-tests/tests/utils/config.py (2)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(51-70)integration-tests/tests/utils/utils.py (3)
unlink(54-75)validate_dir_exists(78-85)validate_file_exists(88-95)
integration-tests/tests/test_package_start.py (3)
integration-tests/tests/utils/config.py (1)
PackageInstance(132-181)integration-tests/tests/utils/package_utils.py (2)
is_package_running(180-213)is_running_mode_correct(216-231)integration-tests/tests/fixtures/package_instance_fixtures.py (1)
clp_package(21-45)
⏰ 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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (9)
integration-tests/pyproject.toml (1)
77-78: LGTM!The local path configuration for
clp-py-utilsenables integration tests to import from the adjacent component package during development.integration-tests/tests/fixtures/package_config_fixtures.py (1)
50-70: LGTM!The fixture correctly creates a temporary config, yields it for tests, and cleans up the temp file on teardown.
integration-tests/tests/utils/config.py (3)
88-118: LGTM with note on frozen dataclass pattern.The
__post_init__correctly validates package structure, initializes cache directories, and setstemp_config_file_pathas a placeholder (with the actual file written by the fixture). Usingobject.__setattr__for frozen dataclasses is acceptable here since initialization happens in__post_init__.
147-163: LGTM!The
__post_init__correctly validates that the log directory and required files exist, and retrieves the instance ID with proper error handling.
165-181: LGTM!The instance ID validation with regex
[0-9a-fA-F]{4}correctly enforces the 4-hex-digit format, with clear error messages for file read failures and invalid formats.integration-tests/tests/utils/package_utils.py (4)
38-54: LGTM!Reusing component names from
clp_py_utils.clp_configand transforming them to container basenames avoids duplication and ensures consistency.
56-69: LGTM!The mode configurations correctly map
clp-textandclp-jsonto their respective storage and query engine pairs, enabling parameterized testing across modes.
180-213: LGTM!The function correctly validates all components are running by checking Docker containers, accumulating issues, and returning a descriptive failure message when problems are found.
216-236: LGTM!The mode verification correctly reads the shared config written by the package, validates it as a
CLPConfig, and compares the derived mode with the intended mode.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
37-44: Redundant path assignment already handled byPackageConfig.__post_init__.Line 44 uses
object.__setattr__to assigntemp_config_file_path, butPackageConfig.__post_init__(lines 119-123 inconfig.py) already sets this field totemp_config_dir / f"clp-config-{mode_name}.yml". Sincewrite_temp_config_filewrites to that exact path and returns it, Line 44 is redundant and bypasses frozen immutability unnecessarily.Apply this diff to remove the redundant assignment:
mode_name: str = request.param logger.info("Creating a temporary config file for the %s package...", mode_name) package_config = _build_package_config_for_mode(mode_name) - clp_config_obj = get_clp_config_from_mode(mode_name) - temp_config_file_path: Path = write_temp_config_file( + write_temp_config_file( clp_config=clp_config_obj, temp_config_dir=package_config.temp_config_dir, mode_name=mode_name, ) - object.__setattr__(package_config, "temp_config_file_path", temp_config_file_path) return package_configWait, I need to check the actual code more carefully. Let me re-read:
Lines 20-46 show
_build_package_config_for_mode. Lines 37-44 show:# Write the temporary config file that the package will use. clp_config_obj = get_clp_config_from_mode(mode_name) temp_config_file_path: Path = write_temp_config_file( clp_config=clp_config_obj, temp_config_dir=package_config.temp_config_dir, mode_name=mode_name, ) object.__setattr__(package_config, "temp_config_file_path", temp_config_file_path)So the issue is that line 44 overwrites what post_init already set. Let me fix my diff:
# Write the temporary config file that the package will use. clp_config_obj = get_clp_config_from_mode(mode_name) - temp_config_file_path: Path = write_temp_config_file( + write_temp_config_file( clp_config=clp_config_obj, temp_config_dir=package_config.temp_config_dir, mode_name=mode_name, ) - object.__setattr__(package_config, "temp_config_file_path", temp_config_file_path) return package_configintegration-tests/tests/fixtures/package_instance_fixtures.py (1)
32-50: PotentialNameErrorifPackageInstanceconstruction fails.If Line 35 raises an exception, the
finallyblock at Line 45 will attempt to referenceinstance, which is undefined, causing aNameErrorthat masks the original exception.Apply this diff to initialize
instancebefore thetryblock:start_clp_package(clp_config) + instance = None try: instance = PackageInstance(package_config=clp_config) instance_id = instance.clp_instance_id logger.info( "An instance of the %s package was started successfully. Its instance ID is '%s'", mode_name, instance_id, ) yield instance finally: logger.info("Now stopping the %s package...", mode_name) if instance is not None: stop_clp_package(instance) else: # This means setup failed after start; fall back to calling stop script directly subprocess.run([str(clp_config.stop_script_path)], check=True) logger.info("The %s package was stopped successfully.", mode_name)integration-tests/tests/utils/package_utils.py (2)
124-138: Consider capturing subprocess output for debugging.The function runs the start script but doesn't capture
stdout/stderr. If startup fails, diagnostic output from the script will be lost, making debugging more difficult.Apply this diff to capture and include output in the error message:
def start_clp_package(package_config: PackageConfig) -> None: """Start an instance of the CLP package.""" start_script_path = package_config.start_script_path try: # fmt: off start_cmd = [ str(start_script_path), "--config", str(package_config.temp_config_file_path) ] # fmt: on - subprocess.run(start_cmd, check=True) - except Exception as e: - err_msg = f"Failed to start an instance of the {package_config.mode_name} package." + subprocess.run(start_cmd, check=True, capture_output=True, text=True) + except subprocess.CalledProcessError as e: + err_msg = ( + f"Failed to start an instance of the {package_config.mode_name} package.\n" + f"stdout: {e.stdout}\nstderr: {e.stderr}" + ) + raise RuntimeError(err_msg) from e + except Exception as e: + err_msg = f"Failed to start an instance of the {package_config.mode_name} package." raise RuntimeError(err_msg) from e
141-154: Consider capturing subprocess output and ensure path type consistency.Similar to
start_clp_package, this function doesn't capture output, making debugging failed stops more difficult. Additionally, Line 148 passesstop_script_pathdirectly, but for consistency withstart_clp_package, it should be converted to a string.Apply this diff:
def stop_clp_package(instance: PackageInstance) -> None: """Stop an instance of the CLP package.""" package_config = instance.package_config stop_script_path = package_config.stop_script_path try: # fmt: off stop_cmd = [ - stop_script_path + str(stop_script_path) ] # fmt: on - subprocess.run(stop_cmd, check=True) - except Exception as e: - err_msg = f"Failed to stop an instance of the {package_config.mode_name} package." + subprocess.run(stop_cmd, check=True, capture_output=True, text=True) + except subprocess.CalledProcessError as e: + err_msg = ( + f"Failed to stop an instance of the {package_config.mode_name} package.\n" + f"stdout: {e.stdout}\nstderr: {e.stderr}" + ) + raise RuntimeError(err_msg) from e + except Exception as e: + err_msg = f"Failed to stop an instance of the {package_config.mode_name} package." raise RuntimeError(err_msg) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
integration-tests/tests/fixtures/package_config_fixtures.py(1 hunks)integration-tests/tests/fixtures/package_instance_fixtures.py(1 hunks)integration-tests/tests/utils/config.py(3 hunks)integration-tests/tests/utils/package_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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: 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.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 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.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/fixtures/package_instance_fixtures.pyintegration-tests/tests/fixtures/package_config_fixtures.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.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/fixtures/package_instance_fixtures.pyintegration-tests/tests/fixtures/package_config_fixtures.py
📚 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/utils/config.pyintegration-tests/tests/utils/package_utils.py
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T22:07:04.953Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:121-123
Timestamp: 2025-08-20T22:07:04.953Z
Learning: For the CLP integration tests codebase, do not suggest deriving tarball filenames from URLs instead of hard-coding ".tar.gz" extension. The user has explicitly rejected this suggestion.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/config.pyintegration-tests/tests/utils/package_utils.py
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T15:23:42.507Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:59-62
Timestamp: 2025-09-02T15:23:42.507Z
Learning: In the CoreConfig class (integration-tests/tests/utils/config.py), clp_binary_path and clp_s_binary_path are decorated with property, so they should be accessed as attributes (without parentheses) rather than method calls.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-03T13:33:27.460Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-07-03T13:33:27.460Z
Learning: In the CLP codebase, `from __future__ import annotations` imports are used to enable Python pipe notation for union types (e.g., `int | str`) in Python versions before 3.10, and should not be considered unused imports.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
integration-tests/tests/utils/package_utils.py
🧬 Code graph analysis (4)
integration-tests/tests/utils/config.py (3)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(50-69)components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(613-853)integration-tests/tests/utils/utils.py (3)
unlink(54-75)validate_dir_exists(78-85)validate_file_exists(88-95)
integration-tests/tests/utils/package_utils.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(50-69)components/clp-py-utils/clp_py_utils/clp_config.py (4)
CLPConfig(613-853)Package(136-160)QueryEngine(112-115)StorageEngine(96-98)integration-tests/tests/utils/config.py (4)
PackageConfig(71-133)PackageInstance(137-190)start_script_path(126-128)stop_script_path(131-133)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)
integration-tests/tests/fixtures/package_instance_fixtures.py (3)
integration-tests/tests/utils/config.py (3)
PackageConfig(71-133)PackageInstance(137-190)stop_script_path(131-133)integration-tests/tests/utils/package_utils.py (2)
start_clp_package(124-138)stop_clp_package(141-154)integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(50-69)
integration-tests/tests/fixtures/package_config_fixtures.py (3)
integration-tests/tests/utils/config.py (1)
PackageConfig(71-133)integration-tests/tests/utils/package_utils.py (2)
get_clp_config_from_mode(73-80)write_temp_config_file(101-121)integration-tests/tests/utils/utils.py (1)
get_env_var(11-21)
⏰ 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). (2)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
38-46: Remove redundant frozen dataclass mutation.
PackageConfig.__post_init__(lines 122-126 inconfig.py) already setstemp_config_file_pathtotemp_config_dir / f"clp-config-{mode_name}.yml". Sincewrite_temp_config_filereturns the identical path (line 128 inpackage_utils.py), Line 45'sobject.__setattr__overwrites the field with the same value—this is redundant.Apply this diff to eliminate the unnecessary mutation:
# Write the temporary config file that the package will use. clp_config_obj = build_config() - temp_config_file_path: Path = write_temp_config_file( + write_temp_config_file( clp_config=clp_config_obj, temp_config_dir=package_config.temp_config_dir, mode_name=mode_name, ) - object.__setattr__(package_config, "temp_config_file_path", temp_config_file_path) return package_configintegration-tests/tests/utils/package_utils.py (2)
117-137: Fix typo in docstring.Line 123: "CLPCongig" should be "CLPConfig".
Apply this diff:
""" - Writes a temporary config file to `temp_config_dir` for a CLPCongig object. Returns the path to + Writes a temporary config file to `temp_config_dir` for a CLPConfig object. Returns the path to the temporary file on success. """
140-170: Capture subprocess output for debugging.Both
start_clp_package(lines 140-154) andstop_clp_package(lines 157-170) run scripts without capturing output. When these operations fail, diagnostic messages from the scripts are lost.Apply this diff to capture and include output in error messages:
def start_clp_package(package_config: PackageConfig) -> None: """Start an instance of the CLP package.""" start_script_path = package_config.start_script_path try: # fmt: off start_cmd = [ str(start_script_path), "--config", str(package_config.temp_config_file_path) ] # fmt: on - subprocess.run(start_cmd, check=True) + subprocess.run(start_cmd, check=True, capture_output=True, text=True) - except Exception as e: - err_msg = f"Failed to start an instance of the {package_config.mode_name} package." + except subprocess.CalledProcessError as e: + err_out = (e.stderr or e.stdout or "").strip() + err_msg = f"Failed to start an instance of the {package_config.mode_name} package. Output: {err_out}" raise RuntimeError(err_msg) from e def stop_clp_package(instance: PackageInstance) -> None: """Stop an instance of the CLP package.""" package_config = instance.package_config stop_script_path = package_config.stop_script_path try: # fmt: off stop_cmd = [ stop_script_path ] # fmt: on - subprocess.run(stop_cmd, check=True) + subprocess.run(stop_cmd, check=True, capture_output=True, text=True) - except Exception as e: - err_msg = f"Failed to stop an instance of the {package_config.mode_name} package." + except subprocess.CalledProcessError as e: + err_out = (e.stderr or e.stdout or "").strip() + err_msg = f"Failed to stop an instance of the {package_config.mode_name} package. Output: {err_out}" raise RuntimeError(err_msg) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
integration-tests/tests/fixtures/package_config_fixtures.py(1 hunks)integration-tests/tests/utils/config.py(3 hunks)integration-tests/tests/utils/package_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
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.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
📚 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/fixtures/package_config_fixtures.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/utils/package_utils.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/fixtures/package_config_fixtures.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/utils/package_utils.py
📚 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/utils/config.pyintegration-tests/tests/utils/package_utils.py
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T22:07:04.953Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:121-123
Timestamp: 2025-08-20T22:07:04.953Z
Learning: For the CLP integration tests codebase, do not suggest deriving tarball filenames from URLs instead of hard-coding ".tar.gz" extension. The user has explicitly rejected this suggestion.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/config.pyintegration-tests/tests/utils/package_utils.py
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T15:23:42.507Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:59-62
Timestamp: 2025-09-02T15:23:42.507Z
Learning: In the CoreConfig class (integration-tests/tests/utils/config.py), clp_binary_path and clp_s_binary_path are decorated with property, so they should be accessed as attributes (without parentheses) rather than method calls.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-03T13:33:27.460Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-07-03T13:33:27.460Z
Learning: In the CLP codebase, `from __future__ import annotations` imports are used to enable Python pipe notation for union types (e.g., `int | str`) in Python versions before 3.10, and should not be considered unused imports.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
integration-tests/tests/utils/package_utils.py
🧬 Code graph analysis (3)
integration-tests/tests/fixtures/package_config_fixtures.py (3)
integration-tests/tests/utils/config.py (1)
PackageConfig(71-136)integration-tests/tests/utils/package_utils.py (1)
write_temp_config_file(117-137)integration-tests/tests/utils/utils.py (1)
get_env_var(11-21)
integration-tests/tests/utils/config.py (3)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(51-70)components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(613-853)integration-tests/tests/utils/utils.py (3)
unlink(54-75)validate_dir_exists(78-85)validate_file_exists(88-95)
integration-tests/tests/utils/package_utils.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(51-70)components/clp-py-utils/clp_py_utils/clp_config.py (4)
CLPConfig(613-853)Package(136-160)QueryEngine(112-115)StorageEngine(96-98)integration-tests/tests/utils/config.py (4)
PackageConfig(71-136)PackageInstance(140-193)start_script_path(129-131)stop_script_path(134-136)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)
⏰ 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). (2)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
Contributor
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/package_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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: 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.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 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/package_utils.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/package_utils.py
📚 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/utils/package_utils.py
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/package_utils.py
🧬 Code graph analysis (1)
integration-tests/tests/utils/package_utils.py (4)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(51-70)components/clp-py-utils/clp_py_utils/clp_config.py (5)
CLPConfig(613-853)Package(136-160)QueryEngine(112-115)StorageEngine(96-98)get_deployment_type(804-808)integration-tests/tests/utils/config.py (4)
PackageConfig(71-136)PackageInstance(140-193)start_script_path(129-131)stop_script_path(134-136)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)
⏰ 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). (4)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build (macos-15)
🔇 Additional comments (5)
integration-tests/tests/utils/package_utils.py (5)
89-96: LGTM!The function correctly retrieves the configuration factory from
CLP_MODE_CONFIGSand provides clear error messaging for unsupported modes.
99-114: LGTM!The function demonstrates robust error handling for YAML parsing and file I/O, with clear error messages and appropriate type validation.
117-137: LGTM!The function uses an atomic write pattern (writing to a temporary file then renaming), which is excellent practice for preventing partial writes or corruption.
173-207: LGTM!The function thoroughly validates that all required component containers exist and are running, providing detailed diagnostic information when issues are found.
225-248: LGTM with dependency on signature fix.The function correctly loads and validates the running configuration against the intended configuration. Ensure the
_compute_mode_signaturefunction is fixed (as noted in the previous comment) so that the signature comparison captures all mode-defining properties.
Contributor
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 (4)
integration-tests/pyproject.toml(3 hunks)integration-tests/tests/fixtures/package_config_fixtures.py(1 hunks)integration-tests/tests/utils/config.py(3 hunks)integration-tests/tests/utils/package_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (22)
📓 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: 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.
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: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
📚 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/fixtures/package_config_fixtures.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/pyproject.tomlintegration-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/fixtures/package_config_fixtures.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/pyproject.tomlintegration-tests/tests/utils/config.py
📚 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/utils/package_utils.pyintegration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T08:36:38.391Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/__init__.py:1-1
Timestamp: 2025-08-20T08:36:38.391Z
Learning: In the CLP project, __init__.py files should include short module-level docstrings describing the package purpose, as required by their ruff linting configuration, rather than being left empty.
Applied to files:
integration-tests/tests/utils/package_utils.pyintegration-tests/pyproject.toml
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
integration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/config.py
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Applied to files:
integration-tests/tests/utils/package_utils.py
📚 Learning: 2025-10-22T21:02:31.113Z
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).
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-09-26T21:12:21.655Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1338
File: components/clp-py-utils/clp_py_utils/clp_config.py:753-753
Timestamp: 2025-09-26T21:12:21.655Z
Learning: Python linting rules that catch YODA conditions exist but are not enabled for Python modules in the CLP package due to time constraints. The team prefers to avoid YODA conditions in Python for consistency.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-07-03T13:33:27.460Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-07-03T13:33:27.460Z
Learning: In the CLP codebase, `from __future__ import annotations` imports are used to enable Python pipe notation for union types (e.g., `int | str`) in Python versions before 3.10, and should not be considered unused imports.
Applied to files:
integration-tests/pyproject.tomlintegration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T13:35:33.396Z
Learnt from: anlowee
Repo: y-scope/clp PR: 1228
File: tools/deployment/presto-clp/scripts/init.py:298-306
Timestamp: 2025-09-02T13:35:33.396Z
Learning: In the CLP codebase, all configuration values loaded from YAML files are string values, so functions that retrieve config values should use Optional[str] rather than Optional[Any] for type hints.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-08-24T17:10:44.565Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: taskfiles/lint.yaml:780-781
Timestamp: 2025-08-24T17:10:44.565Z
Learning: The CLP repository is gradually migrating from Black to Ruff for Python formatting. Some modules haven't been set up for Ruff yet, so there's intentional mixed usage of Black and Ruff formatters during this transition period. This should not be flagged as an inconsistency in PR reviews.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-10-27T04:05:01.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1507
File: taskfiles/deps/lock.yaml:66-76
Timestamp: 2025-10-27T04:05:01.560Z
Learning: In the CLP project, the `uv` CLI tool is documented as a prerequisite in docs/src/dev-docs/building-package.md and is not installed via a task dependency like the rust toolchain. UV is expected to be available in the environment rather than being provisioned by the taskfile system.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-08-29T07:31:24.032Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1271
File: components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh:41-43
Timestamp: 2025-08-29T07:31:24.032Z
Learning: In CLP's build tool installation scripts, uv version constraints should use lower bound constraints (uv>=0.8) rather than exact version pinning, following the same philosophy used for other build tools to accommodate platform differences while ensuring minimum required functionality.
Applied to files:
integration-tests/pyproject.toml
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-08-20T22:07:04.953Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:121-123
Timestamp: 2025-08-20T22:07:04.953Z
Learning: For the CLP integration tests codebase, do not suggest deriving tarball filenames from URLs instead of hard-coding ".tar.gz" extension. The user has explicitly rejected this suggestion.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
integration-tests/tests/utils/config.py
📚 Learning: 2025-09-02T15:23:42.507Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/test_identity_transformation.py:59-62
Timestamp: 2025-09-02T15:23:42.507Z
Learning: In the CoreConfig class (integration-tests/tests/utils/config.py), clp_binary_path and clp_s_binary_path are decorated with property, so they should be accessed as attributes (without parentheses) rather than method calls.
Applied to files:
integration-tests/tests/utils/config.py
🧬 Code graph analysis (3)
integration-tests/tests/fixtures/package_config_fixtures.py (2)
integration-tests/tests/utils/config.py (1)
PackageConfig(73-153)integration-tests/tests/utils/utils.py (2)
get_env_var(13-23)unlink(81-102)
integration-tests/tests/utils/package_utils.py (5)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(613-853)integration-tests/tests/utils/clp_mode_utils.py (2)
compute_mode_signature(75-87)get_clp_config_from_mode(90-97)integration-tests/tests/utils/config.py (2)
PackageConfig(73-153)PackageInstance(157-210)integration-tests/tests/utils/docker_utils.py (2)
inspect_container_state(48-73)list_prefixed_containers(17-45)integration-tests/tests/utils/utils.py (1)
load_yaml_to_dict(56-78)
integration-tests/tests/utils/config.py (3)
integration-tests/tests/fixtures/package_config_fixtures.py (1)
clp_config(37-56)components/clp-py-utils/clp_py_utils/clp_config.py (1)
CLPConfig(613-853)integration-tests/tests/utils/utils.py (3)
unlink(81-102)validate_dir_exists(105-112)validate_file_exists(115-122)
🔇 Additional comments (3)
integration-tests/pyproject.toml (3)
38-40: Good: Per-module mypy overrides properly implemented.The use of
[[tool.mypy.overrides]]with module patternclp_py_utils.*correctly addresses the prior feedback to avoid a globalignore_missing_imports = truesetting. This preserves strict type checking for local test code while selectively disabling it only for the untyped external module—a best practice that balances safety with pragmatism.
80-81: Path verification confirms correct resolution.The uv source mapping in
integration-tests/pyproject.tomlcorrectly resolves tocomponents/clp-py-utils. The relative path../components/clp-py-utilssuccessfully points to the existing package directory and requires no changes.
20-20: All dependency constraints are correctly configured.The concerns from the original review are resolved:
clp-py-utilslacks a version constraint by design—this is appropriate because it's sourced from a local path via uv sources (line 81), following the standard monorepo development pattern.- PyYAML 6.0.3 (latest) is compatible with Python 3.10+, and types-PyYAML 6.0.12 is compatible with Python 3.10+. The project specifies
requires-python = ">=3.10", ensuring full compatibility.- The relative path
../components/clp-py-utilscorrectly resolves from the integration-tests directory.- The mypy overrides (lines 38–40) appropriately implement selective type checking, addressing previous feedback about avoiding global
ignore_missing_imports = true.
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.
Description
This PR builds on the infrastructure introduced in PR #1100 and PR #1549 by introducing a parametrized test called
test_clp_packagethat ensures two key configurations of the clp package (clp-textandclp-json) spin up and tear down successfully.test_clp_packageuses the contents of a dictionary calledCLP_MODE_CONFIGSto spin up each mode in its proper configuration.The tests are supported by new infrastructure that will make the future expansion of package integration testing easier in two key ways:
"clp-json-presto","clp-json-s3", etc.) will only require adding a description of their configurations toCLP_MODE_CONFIGS.In addition, this PR introduces
Checklist
breaking change.
Validation performed
Ran the following command in the
/clprepo:The package was built successfully, and each of the two tests (
clp-textandclp-json) pass.In addition, ran the following command from
clp/integration-tests:uv run python -m pytest -m package -k 'clp-json'The
clp-jsontest ran independently and passed.Summary by CodeRabbit
Tests
Chores