feat(integration-tests): Shift to modular design for package integration tests; restructure test directories; add sample test functions to test_clp_json.
#1843
Conversation
WalkthroughSplit test configuration into PackageModeConfig and PackageTestConfig, replace package_config usages across fixtures and utilities, remove centralized CLP mode factories, add clp-json and clp-text integration tests, introduce new pytest markers, and delete the old centralized startup test. (50 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.14.14)integration-tests/tests/package_tests/clp_json/test_clp_json.py�[1;31mruff failed�[0m integration-tests/tests/package_tests/clp_text/test_clp_text.py�[1;31mruff failed�[0m integration-tests/tests/utils/config.py�[1;31mruff failed�[0m
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI Agents
In @integration-tests/.pytest.ini:
- Around line 3-6: Remove the inactive commented pytest options so the config is
clean: either delete the two commented lines containing "; --capture=no" and ";
-rA" if those flags are no longer needed, or, if they should be active,
uncomment the lines to restore "--capture=no" and "-rA"; update the pytest
config where the lines with "--capture=no" and "-rA" appear accordingly.
- Line 26: The pytest marker description for the 'search' marker is
inconsistent; update the marker line that currently reads 'search: search tests'
to follow the project's standard pattern (e.g., 'search: mark tests that
exercise search functionality' or similar) so it matches other marker
descriptions and improves clarity.
In @integration-tests/tests/package_tests/clp_json/test_clp_json.py:
- Around line 56-69: The placeholder test test_clp_json_compression_multifile
currently only calls validate_package_instance and asserts True; replace the
TODO by invoking the clp-json package via the fixt_package_instance to compress
the json-multifile dataset, capture the produced output (files/paths/metadata),
and add assertions that verify compression occurred and is correct (e.g.,
expected compressed file count/names, file sizes reduced, and checksums or
round-trip decompression equality against original data); use
validate_package_instance for setup, then call the package runner/handler on the
json-multifile input, collect outputs and assert expected outcomes instead of
the dummy assert, and log success with logger.info when checks pass.
In @integration-tests/tests/package_tests/clp_text/test_clp_text.py:
- Around line 52-53: The log message in test_clp_text.py uses an f-string
(log_msg = f"{CLP_TEXT_MODE.mode_name} is running successfully." followed by
logger.info(log_msg)) which is inconsistent with test_clp_json.py's hardcoded
log string; pick one style and standardize both tests. Update either
test_clp_text.py to use the same hardcoded string used in test_clp_json.py
(replacing log_msg and the f-string reference to CLP_TEXT_MODE.mode_name) or
change test_clp_json.py to use the CLP_TEXT_MODE.mode_name f-string pattern, and
ensure the logging call remains logger.info(...).
In @integration-tests/tests/utils/asserting_utils.py:
- Line 56: Replace the unnecessary use of object.__setattr__ when setting
instance_validated on package_instance: since PackageInstance is not frozen,
assign directly to the attribute (i.e., use package_instance.instance_validated
= True) wherever object.__setattr__(package_instance, "instance_validated",
True) appears (check functions in asserting_utils.py that manipulate
package_instance).
In @integration-tests/tests/utils/config.py:
- Line 123: Replace the non-idiomatic empty-parentheses dataclass decorators by
removing the parentheses: change each occurrence of @dataclass() to @dataclass
(the three decorator instances currently written as @dataclass() in the file) so
the dataclass declarations use the canonical form; no other behavior or imports
need to change.
- Around line 123-135: The PackageModeConfig dataclass declares component_list:
list[str] which is mutable; if you introduce a default for it later you must
avoid shared-state bugs by using dataclasses.field(default_factory=list) for
component_list in the PackageModeConfig class; either add component_list:
list[str] = field(default_factory=list) (importing field from dataclasses) or
explicitly document/enforce that callers must always pass a list to the
PackageModeConfig constructor so no implicit shared mutable default is used.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
integration-tests/.pytest.iniintegration-tests/tests/conftest.pyintegration-tests/tests/fixtures/package_instance.pyintegration-tests/tests/fixtures/package_test_config.pyintegration-tests/tests/package_tests/clp_json/__init__.pyintegration-tests/tests/package_tests/clp_json/test_clp_json.pyintegration-tests/tests/package_tests/clp_text/__init__.pyintegration-tests/tests/package_tests/clp_text/test_clp_text.pyintegration-tests/tests/test_package_start.pyintegration-tests/tests/utils/asserting_utils.pyintegration-tests/tests/utils/clp_mode_utils.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/utils/package_utils.py
💤 Files with no reviewable changes (1)
- integration-tests/tests/test_package_start.py
🧰 Additional context used
🧠 Learnings (7)
📓 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: 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.
📚 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/package_tests/clp_json/test_clp_json.pyintegration-tests/tests/package_tests/clp_json/__init__.pyintegration-tests/tests/conftest.pyintegration-tests/.pytest.iniintegration-tests/tests/package_tests/clp_text/test_clp_text.pyintegration-tests/tests/package_tests/clp_text/__init__.pyintegration-tests/tests/fixtures/package_instance.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/package_tests/clp_json/test_clp_json.pyintegration-tests/tests/package_tests/clp_json/__init__.pyintegration-tests/tests/conftest.pyintegration-tests/.pytest.iniintegration-tests/tests/package_tests/clp_text/__init__.pyintegration-tests/tests/utils/package_utils.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/package_tests/clp_json/__init__.pyintegration-tests/tests/package_tests/clp_text/test_clp_text.pyintegration-tests/tests/package_tests/clp_text/__init__.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/package_tests/clp_json/__init__.pyintegration-tests/tests/package_tests/clp_text/__init__.pyintegration-tests/tests/utils/package_utils.py
📚 Learning: 2025-11-10T05:19:56.600Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Applied to files:
integration-tests/tests/utils/clp_mode_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/clp_mode_utils.py
🧬 Code graph analysis (6)
integration-tests/tests/package_tests/clp_json/test_clp_json.py (3)
integration-tests/tests/utils/asserting_utils.py (1)
validate_package_instance(40-56)integration-tests/tests/utils/config.py (1)
PackageInstance(172-226)integration-tests/tests/fixtures/package_instance.py (1)
fixt_package_instance(18-39)
integration-tests/tests/fixtures/package_test_config.py (4)
integration-tests/tests/utils/config.py (4)
PackageModeConfig(124-134)PackagePathConfig(67-120)PackageTestConfig(138-168)temp_config_file_path(155-157)integration-tests/tests/utils/port_utils.py (1)
assign_ports_from_base(39-61)integration-tests/tests/fixtures/path_configs.py (1)
fixt_package_path_config(28-35)integration-tests/tests/utils/utils.py (1)
unlink(107-128)
integration-tests/tests/package_tests/clp_text/test_clp_text.py (3)
integration-tests/tests/utils/asserting_utils.py (1)
validate_package_instance(40-56)integration-tests/tests/utils/config.py (2)
PackageInstance(172-226)PackageModeConfig(124-134)integration-tests/tests/fixtures/package_instance.py (1)
fixt_package_instance(18-39)
integration-tests/tests/fixtures/package_instance.py (3)
integration-tests/tests/utils/config.py (2)
PackageTestConfig(138-168)PackageInstance(172-226)integration-tests/tests/utils/package_utils.py (2)
start_clp_package(9-26)stop_clp_package(29-46)integration-tests/tests/fixtures/package_test_config.py (1)
fixt_package_test_config(12-45)
integration-tests/tests/utils/package_utils.py (1)
integration-tests/tests/utils/config.py (3)
PackageTestConfig(138-168)start_script_path(113-115)temp_config_file_path(155-157)
integration-tests/tests/utils/asserting_utils.py (1)
integration-tests/tests/utils/config.py (1)
PackageInstance(172-226)
⏰ 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: check-generated
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (15)
integration-tests/tests/package_tests/clp_json/__init__.py (1)
1-1: LGTM!The module-level docstring follows the project's ruff linting requirements for
__init__.pyfiles. Based on learnings, this is the expected convention.integration-tests/tests/package_tests/clp_text/__init__.py (1)
1-1: LGTM!Consistent with project conventions for
__init__.pyfiles. Based on learnings, the module-level docstring is required.integration-tests/tests/conftest.py (1)
10-10: LGTM!The plugin path update correctly aligns with the fixture module rename from
package_configtopackage_test_config.integration-tests/tests/package_tests/clp_json/test_clp_json.py (1)
22-32: LGTM!The
CLP_JSON_MODEconfiguration correctly usesStorageEngine.CLP_SandQueryEngine.CLP_S, which is a valid combination per thePackagemodel validator. Including the API server component aligns with the JSON package requirements.integration-tests/tests/package_tests/clp_text/test_clp_text.py (1)
22-36: LGTM!The
CLP_TEXT_MODEconfiguration correctly usesStorageEngine.CLPandQueryEngine.CLP. Explicitly settingapi_server=Noneandlog_ingestor=Noneclearly indicates these components are not used in text mode.integration-tests/tests/fixtures/package_test_config.py (3)
11-15: LGTM!The module-scoped fixture is appropriate for test isolation per mode. The rename from
fixt_package_configtofixt_package_test_configand updated return type correctly reflect the new configuration structure.
23-24: LGTM!Extracting
mode_configfromrequest.paramand accessingclp_configfrom it aligns with the newPackageModeConfigstructure. The type annotation provides clear documentation.
35-45: LGTM!The fixture correctly:
- Constructs
PackageTestConfigafter port assignment (ensuring ports are set inclp_configbefore the temp file is written in__post_init__)- Yields the config for test usage
- Cleans up the temp config file with
missing_ok=Truefor safe teardownintegration-tests/tests/utils/clp_mode_utils.py (1)
34-52: LGTM! Component list simplification aligns with modular design.The comment update clarifies the alignment requirement with package operating modes, and the retained constants (
CLP_BASE_COMPONENTSandCLP_API_SERVER_COMPONENT) remain appropriately defined for use across the test suite.integration-tests/tests/utils/package_utils.py (1)
4-46: LGTM! Clean parameter rename throughout.The rename from
package_configtopackage_test_configis consistently applied across both functions and their docstrings, aligning with the broader refactoring to separate test configuration from mode configuration.integration-tests/tests/utils/config.py (2)
184-186: Good addition of validation guard.The
instance_validatedflag enables the validation guard pattern invalidate_package_instance, preventing redundant validation checks. This is a sensible optimization for the modular test design.
137-168: Verify mutability is intentional for test configuration.Both
PackageModeConfigandPackageTestConfigare mutable (not frozen), unlike most other configuration classes in this file which usefrozen=True.If these configurations are modified during tests, this is intentional. However, if they should remain immutable after initialization, consider adding
frozen=Trueto prevent accidental modification and improve thread safety.Do these configurations need to be modified after initialization during test execution, or should they be immutable like the other config classes?
integration-tests/tests/utils/asserting_utils.py (1)
40-57: Well-designed validation guard pattern.The renamed
validate_package_instancefunction properly checksinstance_validatedbefore performing validations, preventing redundant checks. The delegation to private helper functions (_validate_package_runningand_validate_running_mode_correct) improves code organization.integration-tests/tests/fixtures/package_instance.py (2)
9-39: LGTM! Clean fixture refactoring with proper error handling.The fixture correctly uses
PackageTestConfig, properly starts and stops the package instance, and provides helpful error messages when startup fails. The teardown in thefinallyblock ensures cleanup occurs even on failure.
17-17: The module-scoped fixture is properly designed for safe sharing. The fixture implements robust cleanup with afinallyblock that callsstop_clp_package()regardless of test outcome, ensuring the package instance is properly stopped after all tests in the module complete. The three tests using this fixture are startup tests that do not appear to modify shared package state, eliminating concerns about test interference. This scope is appropriate for a service fixture that should start and stop once per module rather than per test.
integration-tests/tests/package_tests/clp_text/test_clp_text.py
Outdated
Show resolved
Hide resolved
quinntaylormitchell
changed the title
test_clp_json.test_clp_json.
Bill-hbrhbr
left a comment
Bill-hbrhbr
left a comment
There was a problem hiding this comment.
Two TLDR comments:
- Explain the interworking of
fixt_package_instance->fixt_package_test_config->PackageModeConfiginitialization chaining in the docstring offixt_package_test_config. - What's the future outlook for the
fixt_package_test_configclass?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@integration-tests/tests/fixtures/package_instance.py`:
- Around line 28-42: The teardown always calls stop_clp_package even if
start_clp_package failed, which can raise and hide the original error; modify
the fixture to track whether start_clp_package succeeded (e.g., set a local
started flag after start_clp_package returns and/or after constructing
PackageInstance) and only call stop_clp_package in the finally block when that
flag is true; reference start_clp_package, stop_clp_package, PackageInstance and
fixt_package_test_config so you add the flag in the same scope and conditionally
run stop_clp_package to avoid stopping a package that never started.
In `@integration-tests/tests/fixtures/package_test_config.py`:
- Around line 32-37: The current conversion of base_port_string (from
request.config.getoption("--base-port")) only catches ValueError, so a missing
option causing int(None) raises TypeError and bypasses the handler; update the
error handling in the base_port assignment (where base_port_string, base_port
are used) to catch both ValueError and TypeError (e.g., except (ValueError,
TypeError) as err) and re-raise a ValueError with the same user-friendly message
so omitted or malformed --base-port options produce the intended error message.
Co-authored-by: Bingran Hu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@integration-tests/tests/utils/config.py`:
- Around line 147-152: Validate the base_port value in the dataclass
__post_init__ before calling _write_temp_config_file: check that self.base_port
is an int within 1–65535 and if not raise a ValueError with a clear message
including the invalid value (e.g., "base_port must be between 1 and 65535, got
X"); perform this check at the start of __post_init__ so invalid configs fail
fast.
- Around line 123-134: Add a __post_init__ to PackageModeConfig that validates
mode_name is a non-empty, non-whitespace string (e.g., strip() and check
truthiness) and raise ValueError with a clear message if invalid; locate the
dataclass PackageModeConfig and implement the check in its __post_init__ (follow
the same pattern used by IntegrationTestLogs and CompressionTestPathConfig) so
instantiation fails fast for empty mode_name.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
mostly lgtm
i appreciate the efforts into figuring out the requirements and coming up with the abstractions to this sophisticated state. (good job! )
to guide future readers / developers, i feel some high-level docs would be really helpful as we brought up in some discussions in this review. let's work towards that as we get more code merged.
anyways, all is in the right direction and let's push it hard to make the test suite more complete.
integration-tests/tests/package_tests/clp_text/test_clp_text.py
Outdated
Show resolved
Hide resolved
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm. i responded to other followup discussion as well. once we resolve this https://github.com/y-scope/clp/pull/1843/files#r2730289868 , we can merge
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@integration-tests/tests/utils/config.py`:
- Around line 133-134: The dataclass currently declares component_list:
list[str] which allows mutation even with frozen=True; change the type to an
immutable sequence (e.g., component_list: tuple[str, ...]) and update any
constructors/uses to pass/convert tuples (or wrap lists with tuple(...)) so the
frozen dataclass provides true immutability; ensure the dataclass definition and
any code that constructs or iterates over component_list (tests/helpers that
build config objects) are updated to accept the tuple type.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@integration-tests/tests/package_tests/clp_json/test_clp_json.py`:
- Around line 79-85: Replace the copied "compress some dataset" TODO with a
clear, accurate task for preparing data for search: update the comment near the
validate_package_instance call (in test_clp_json) to say something like "prepare
dataset for search (e.g., create or compress dataset as needed) and validate
search correctness" so it no longer duplicates the compression-specific TODO
from test_clp_json_compression and clarifies that compression may be a
prerequisite but the goal is to prepare the dataset for subsequent search
verification.
3 participants
Description
Modularization of the design for the package integration tests will make it easier to work with in the long run. This PR sets up everything needed so that devs will be able to write independent test functions.
Checklist
breaking change.
Validation performed
Ran the following command from
clp/integration-tests:uv run pytest -m 'package and startup'Both the
clp-jsonandclp-textstartup tests passed.Summary by CodeRabbit
Tests
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.