feat(integration-tests): Validate generated package service configuration integrity. by quinntaylormitchell · Pull Request #1660 · y-scope/clp

quinntaylormitchell · 2025-11-25T19:17:10Z

Description

This PR introduces the validate_running_mode_correct function to read the shared config file (clp-package/var/log/.clp-config.yaml, created when the package spins up) and ensure that the package is running in the correct mode.

load_yaml_to_dict loads the yaml in the shared config file into a dictionary, which is then converted to a ClpConfig object with ClpConfig.model_validate. The "signature" of this ClpConfig object is compared to the signature of the ClpConfig object stored in the PackageConfig object for the test run with compare_mode_signatures.

PR dependencies

This PR depends on the following being merged:

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Ran the following command from clp/integration-tests:

uv run pytest -m 'package'

Both the clp-json and clp-text tests passed.

Summary by CodeRabbit

Tests
- Strengthened integration tests with a reusable validator that loads and validates the shared YAML, compares running vs intended mode signatures, and fails on mismatches.
- Added YAML-loading support for test configurations with robust parsing and top-level structure validation.
Chores
- Test setup now discovers and validates the shared configuration file path used during test runs.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

…ig file; address comments.

…er-level clp-config.yml

…-scope#1549 is merged.

…E_CONFIGS other than clp-text and clp-json

…nto one class.

…fter y-scope#1549).

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

integration-tests/tests/utils/asserting_utils.py (1)

71-91: Implementation looks correct.

The function correctly:

Loads and validates the shared config YAML

Retrieves the intended config from the package instance

Compares using the new compare_mode_signatures helper

The design choice to propagate load_yaml_to_dict errors (as documented in the docstring) is valid—it allows callers to distinguish between file/YAML issues and validation issues.

Consider enhancing the mismatch error message. Adding details about what differs would aid debugging, though this was raised in a previous review.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb775d8 and ac069dc.

📒 Files selected for processing (2)

integration-tests/tests/utils/asserting_utils.py (2 hunks)
integration-tests/tests/utils/clp_mode_utils.py (5 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 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).

📚 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/asserting_utils.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/tests/utils/asserting_utils.py

📚 Learning: 2025-10-20T20:19:40.504Z

Learnt from: 20001020ycx
Repo: y-scope/clp PR: 1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.

Applied to files:

integration-tests/tests/utils/asserting_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 (1)

integration-tests/tests/utils/asserting_utils.py (4)

components/clp-py-utils/clp_py_utils/clp_config.py (1)

ClpConfig (779-1060)

integration-tests/tests/utils/clp_mode_utils.py (1)

compare_mode_signatures (98-107)

integration-tests/tests/utils/config.py (1)

PackageInstance (161-214)

integration-tests/tests/utils/utils.py (1)

load_yaml_to_dict (69-93)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: package-image
GitHub Check: lint-check (macos-15)
GitHub Check: build (ubuntu-24.04)
GitHub Check: lint-check (ubuntu-24.04)
GitHub Check: build (macos-15)

🔇 Additional comments (5)

integration-tests/tests/utils/clp_mode_utils.py (4)

4-4: LGTM!

The new imports for Any and BaseModel are appropriate for the recursive type comparison helper.

Also applies to: 24-24

54-69: LGTM!

The reorganization to include GARBAGE_COLLECTOR_COMPONENT_NAME in CLP_BASE_COMPONENTS makes sense, and the maintenance note aligns with the file's purpose.

98-107: LGTM!

Clean wrapper function with appropriate typing. The separation between the public API and private implementation is good.

129-130: LGTM!

The updated docstring clarifies the maintenance relationship with CLP_MODE_CONFIGS.

integration-tests/tests/utils/asserting_utils.py (1)

9-15: LGTM!

The imports are well-organized and all are necessary for the new validate_running_mode_correct function. The duplicate import issue from past reviews has been addressed.

junhaoliao

the rest lgtm. @Bill-hbrhbr may want to take a look as well

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e39a8b4 and 0239742.

📒 Files selected for processing (1)

integration-tests/tests/utils/utils.py (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: package-image
GitHub Check: build (macos-15)
GitHub Check: build (ubuntu-24.04)
GitHub Check: lint-check (ubuntu-24.04)
GitHub Check: lint-check (macos-15)

🔇 Additional comments (2)

integration-tests/tests/utils/utils.py (2)

8-8: LGTM! Imports are appropriate.

The addition of Any and yaml imports are necessary for the new YAML parsing functionality.

Also applies to: 10-10

10-10: No action needed — PyYAML is already declared in integration-tests/pyproject.toml with appropriate version constraints (PyYAML>=6.0.3 and types-PyYAML>=6.0.12.20250915).

Bill-hbrhbr · 2025-12-16T16:45:54Z

@quinntaylormitchell @junhaoliao
Not sure how I would polish the title, my way of phrasing it would be:

feat(integration-tests): Validate the CLP configuration integrity at runtime.

or something in the lines of:

feat(integration-tests): Validate the CLP package is deployed with the intended configuration.

junhaoliao · 2025-12-16T17:00:37Z

@quinntaylormitchell @junhaoliao Not sure how I would polish the title, my way of phrasing it would be:
feat(integration-tests): Validate the CLP configuration integrity at runtime.
or something in the lines of:
feat(integration-tests): Validate the CLP package is deployed with the intended configuration.

how about

feat(integration-tests): Validate generated package service configuration integrity.

…tion integrity. (y-scope#1660) Co-authored-by: Bingran Hu <bingran.hu@yscope.com> Co-authored-by: Junhao Liao <junhao.liao@yscope.com>

quinntaylormitchell added 30 commits October 17, 2025 21:01

Test a generic clp package start up and spin down (default: clp-text).

8e75e15

Merge branch 'main' into feature branch

44ee20e

Expand to clp-text and clp-json; store JSON config pairs in temp conf…

bbe0575

…ig file; address comments.

Merge branch 'main' into feature branch

5cd47f8

Include and employ clp_py_utils.clp_config; refrain from using the us…

c5375eb

…er-level clp-config.yml

Lint.

2be837f

Refactor; move all utility functions to utils files as appropriate.

bf14b5e

Add comments.

27b7f5b

Streamline logger calls.

e041f0e

Deduplicate fixture code.

75672fc

Merge branch 'main' into feature branch

0822a79

Change method of clp_config import; temporary, will change again once y…

2df9515

…-scope#1549 is merged.

Generalize code so that it's better equipped to handle future CLP_MOD…

63f3ae4

…E_CONFIGS other than clp-text and clp-json

Merge branch 'main' into feature branch

0011447

Combine PackageConfig, PackageModeConfig, and PackageInstanceConfig i…

0ff9465

…nto one class.

Employ a 'CLPConfig'-centric approach rather than 'dict'-centric.

cdcfaee

Store list of required components for each mode

53e7343

Generalize the method by which the mode of operation is determined.

796ffe3

Move all mode-related utilities to their own file.

bae12de

Minor error.

9c9ef74

Move load_yaml_to_dict to general utilities file.

0f3dcd5

Add taskfile tasks for package tests.

f9865a8

Address rabbit comments.

865e9ed

Add mariadb_config install for linting purposes (will be unecessary a…

09d5200

…fter y-scope#1549).

Lint YAML.

5fffc32

Merge branch 'main' into feature branch

571f313

Adapt code after y-scope#1549 merge.

e348be0

Merge branch 'main' into feature branch

30f186f

Merge branch 'main' into feature branch

e5da7e9

Merge branch 'main' into feature branch

292ec70

Address Bill's comments.

bb775d8

quinntaylormitchell dismissed Bill-hbrhbr’s stale review via bb775d8 December 10, 2025 19:29

quinntaylormitchell requested review from Bill-hbrhbr and junhaoliao and removed request for junhaoliao December 10, 2025 19:30

Change compute_mode_signature to compare_mode_signatures.

ac069dc

coderabbitai Bot reviewed Dec 12, 2025

View reviewed changes

Comment thread integration-tests/tests/utils/clp_mode_utils.py Outdated

quinntaylormitchell added 2 commits December 12, 2025 15:37

Merge branch 'main' into feature branch

08bd34d

Add log_ingestor=None for clp-text.

ab94382

quinntaylormitchell requested review from junhaoliao and removed request for junhaoliao December 12, 2025 16:25

Merge branch 'main' into feature branch

14361ef

junhaoliao requested changes Dec 15, 2025

View reviewed changes

quinntaylormitchell added 2 commits December 15, 2025 17:42

Address Junhao's comments.

e39a8b4

Address Junhao's comments.

0239742

quinntaylormitchell requested review from Bill-hbrhbr and junhaoliao and removed request for Bill-hbrhbr December 15, 2025 17:47

coderabbitai Bot reviewed Dec 15, 2025

View reviewed changes

Comment thread integration-tests/tests/utils/utils.py

Merge branch 'main' into testing-start-stop-4

677127b

Bill-hbrhbr approved these changes Dec 16, 2025

View reviewed changes

junhaoliao approved these changes Dec 16, 2025

View reviewed changes

Merge branch 'main' into testing-start-stop-4

df9090e

quinntaylormitchell changed the title ~~feat(integration-tests): Validate the CLP package is running in the correct mode of operation.~~ feat(integration-tests): Validate generated package service configuration integrity. Dec 16, 2025

quinntaylormitchell merged commit 71f5d35 into y-scope:main Dec 16, 2025
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(integration-tests): Validate generated package service configuration integrity.#1660

feat(integration-tests): Validate generated package service configuration integrity.#1660
quinntaylormitchell merged 69 commits into
y-scope:mainfrom
quinntaylormitchell:testing-start-stop-4

quinntaylormitchell commented Nov 25, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

junhaoliao left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Bill-hbrhbr commented Dec 16, 2025

Uh oh!

junhaoliao commented Dec 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

quinntaylormitchell commented Nov 25, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR dependencies

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

junhaoliao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bill-hbrhbr commented Dec 16, 2025

Uh oh!

junhaoliao commented Dec 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

quinntaylormitchell commented Nov 25, 2025 •

edited by coderabbitai Bot

Loading