fix(integration-tests): Allow CLP core binary tests to run without depending on the clp package (fixes #1281). by Bill-hbrhbr · Pull Request #1333 · y-scope/clp

Bill-hbrhbr · 2025-09-25T14:59:41Z

Description

Currently, all integration tests—including those only targeting CLP core binaries—require the full CLP package to be built. This happens because PackageConfig.__post_init__ enforces package directory validation for every pytest run, which is unnecessary for core-only tests.

This PR addresses the issue by separating CoreConfig and PackageConfig into standalone fixtures, allowing them to run independently based on test requirements. The tests:integration:core task has been updated to depend on ::core rather than ::package.

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

task clean followed by task tests:integration:core completes successfully.

Summary by CodeRabbit

Tests
- Restructured integration test configuration into separate components for clearer, more modular setup.
- Updated identity transformation tests to use the new core configuration input, improving isolation and readability.
Refactor
- Simplified the integration test configuration surface by removing embedded sub-configurations, reducing coupling and improving maintainability.
Chores
- Updated the integration test task dependency to rely directly on the core component, aligning the workflow with the new configuration structure.

…independent from each other

coderabbitai · 2025-09-25T14:59:50Z

Walkthrough

Separated core and package pytest fixtures, removed core/package fields from IntegrationTestConfig, updated tests and a helper to accept core_config, and changed a CI/task dependency from ::package to ::core.

Changes

Cohort / File(s)	Summary
Fixtures refactor `integration-tests/tests/fixtures/integration_test_config.py`	Replaced a single integration_test_config fixture with three fixtures: `core_config() -> CoreConfig`, `package_config() -> PackageConfig`, and a simplified `integration_test_config()` that sets `test_root_dir` directly.
Config model simplification `integration-tests/tests/utils/config.py`	Removed `core_config: CoreConfig` and `package_config: PackageConfig` fields from `IntegrationTestConfig`; dataclass now exposes only path-related fields (e.g., `test_root_dir`, logs download fields).
Tests updated to new fixtures `integration-tests/tests/test_identity_transformation.py`	Tests now accept `core_config` fixture; replaced usages of `integration_test_config.core_config` and updated helper signature to `_clp_s_compress_and_decompress(core_config: CoreConfig, test_paths: CompressionTestConfig)`.
Task dependency change `taskfiles/tests/integration.yaml`	Changed the task dependency from `::package` to `::core`.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Pytest as pytest
  participant FixCore as core_config (fixture)
  participant FixPkg as package_config (fixture)
  participant FixIT as integration_test_config (fixture)
  participant Test as identity tests
  participant Helper as _clp_s_compress_and_decompress

  Pytest->>FixCore: instantiate CoreConfig
  Pytest->>FixPkg: instantiate PackageConfig
  Pytest->>FixIT: instantiate IntegrationTestConfig (set test_root_dir)
  Pytest->>Test: call test with core_config + integration_test_config
  Test->>Helper: call _clp_s_compress_and_decompress(core_config, test_paths)
  Helper-->>Test: return compression/decompression result
  Test-->>Pytest: assert results

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Optimize integration test dependencies: core tests should only require core binaries #1281 — Matches the fixture split and removal of core/package from IntegrationTestConfig and the change to depend on ::core to allow core-only tests without building the package.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and concisely describes the primary change introduced by the pull request, namely modifying the integration tests so that CLP core binary tests no longer depend on the full CLP package, and it follows conventional commit conventions while referencing the relevant issue.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

taskfiles/tests/integration.yaml (1)
22-26: Remove unused CLP_PACKAGE_DIR from core task env to avoid reintroducing a package dependency.

If core tests truly don’t need the package, drop CLP_PACKAGE_DIR from the env for the core task to prevent accidental coupling or failures when G_PACKAGE_BUILD_DIR is unset.

Suggested YAML (only relevant section shown):
  core:
    deps:
      - task: "::core"
    dir: "{{.G_INTEGRATION_TESTS_DIR}}"
    env:
      CLP_BUILD_DIR: "{{.G_BUILD_DIR}}"
      CLP_CORE_BINS_DIR: "{{.G_CORE_COMPONENT_BUILD_DIR}}"
    cmd: "uv run python -m pytest -m core"

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 761ec12 and 7f58b05.

📒 Files selected for processing (4)

integration-tests/tests/fixtures/integration_test_config.py (1 hunks)
integration-tests/tests/test_identity_transformation.py (9 hunks)
integration-tests/tests/utils/config.py (0 hunks)
taskfiles/tests/integration.yaml (1 hunks)

💤 Files with no reviewable changes (1)

integration-tests/tests/utils/config.py

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: Bill-hbrhbr
PR: y-scope/clp#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
PR: y-scope/clp#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-16T10:24:29.316Z

Learnt from: Bill-hbrhbr
PR: y-scope/clp#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:

taskfiles/tests/integration.yaml

📚 Learning: 2025-07-28T08:33:57.487Z

Learnt from: Bill-hbrhbr
PR: y-scope/clp#1100
File: integration-tests/tests/test_identity_transformation.py:133-137
Timestamp: 2025-07-28T08:33:57.487Z
Learning: In CLP integration tests, the `run_and_assert` utility function should only be used for commands that test CLP package functionality (like clp/clp-s compression/decompression operations). For processing or helper commands (like jq, sort, etc.), direct `subprocess.run` calls should be used instead, as they serve different purposes and don't need the same error handling patterns.

Applied to files:

integration-tests/tests/test_identity_transformation.py

📚 Learning: 2025-09-02T15:23:42.507Z

Learnt from: Bill-hbrhbr
PR: y-scope/clp#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/test_identity_transformation.py

📚 Learning: 2024-11-19T17:30:04.970Z

Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.

Applied to files:

integration-tests/tests/test_identity_transformation.py

📚 Learning: 2025-08-17T16:10:38.722Z

Learnt from: Bill-hbrhbr
PR: y-scope/clp#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/integration_test_config.py

🧬 Code graph analysis (2)

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

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

CoreConfig (15-47)

clp_binary_path (40-42)

CompressionTestConfig (123-154)

clear_test_outputs (151-154)

clp_s_binary_path (45-47)

integration-tests/tests/fixtures/integration_test_config.py (1)

core_config (16-20)

integration-tests/tests/fixtures/integration_test_config.py (2)

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

CoreConfig (15-47)

PackageConfig (51-70)

IntegrationTestConfig (74-91)

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). (14)

GitHub Check: package-image
GitHub Check: ubuntu-jammy-static-linked-bins
GitHub Check: ubuntu-jammy-lint
GitHub Check: ubuntu-jammy-dynamic-linked-bins
GitHub Check: musllinux_1_2-x86_64-static-linked-bins
GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
GitHub Check: centos-stream-9-static-linked-bins
GitHub Check: centos-stream-9-dynamic-linked-bins
GitHub Check: manylinux_2_28-x86_64-static-linked-bins
GitHub Check: build-macos (macos-15, false)
GitHub Check: build-macos (macos-14, true)
GitHub Check: build-macos (macos-15, true)
GitHub Check: build-macos (macos-14, false)

🔇 Additional comments (7)

taskfiles/tests/integration.yaml (1)

20-20: Switching dependency to ::core is correct for decoupling.

This aligns the core test task with the core build, removing the implicit dependency on a complete package.

integration-tests/tests/test_identity_transformation.py (3)

11-12: Importing CoreConfig here is appropriate.

Matches the refactor to inject core_config via fixtures.

41-44: Injecting core_config into tests decouples them from package config.

Good move to consume CoreConfig directly instead of accessing it via integration_test_config.

Also applies to: 95-98

62-62: Approve property access migration No lingering clp_binary_path() or clp_s_binary_path() calls found; properties accessed correctly.

integration-tests/tests/fixtures/integration_test_config.py (3)

16-21: core_config fixture looks good and respects environment-driven paths.

Session scoping and path normalisation are appropriate; CoreConfig will validate at construction.

23-29: package_config fixture is fine; it won’t execute during core-only runs.

Defining it alongside core_config keeps parity for package-focused tests.

31-37: integration_test_config correctly stops wiring in core/package and focuses on paths.

This aligns with the decoupling objective and keeps directory creation centralised.

If the goal is to run core tests without any package artefacts, ensure your core test Task doesn’t require CLP_PACKAGE_DIR (see integration task suggestion). Also confirm CLP_BUILD_DIR is set in CI and local environments that run these tests.

coderabbitai · 2025-09-25T15:12:22Z

 def _clp_s_compress_and_decompress(
-    integration_test_config: IntegrationTestConfig, test_paths: CompressionTestConfig
+    core_config: CoreConfig,
+    test_paths: CompressionTestConfig,
 ) -> None:
    test_paths.clear_test_outputs()
-    bin_path = str(integration_test_config.core_config.clp_s_binary_path)
+    bin_path = str(core_config.clp_s_binary_path)
    src_path = str(test_paths.logs_source_dir)


🧹 Nitpick | 🔵 Trivial

Consider mirroring the clp-s helper for clp to reduce duplication.

You can introduce a _clp_compress_and_decompress(core_config, test_paths) helper (patterned after the clp-s one) and call it from test_clp_identity_transform.

Example helper:

def _clp_compress_and_decompress(core_config: CoreConfig, test_paths: CompressionTestConfig) -> None: test_paths.clear_test_outputs() bin_path = str(core_config.clp_binary_path) src_path = str(test_paths.logs_source_dir) compression_path = str(test_paths.compression_dir) decompression_path = str(test_paths.decompression_dir) compression_cmd = [ bin_path, "c", "--progress", "--remove-path-prefix", src_path, compression_path, src_path ] run_and_assert(compression_cmd) run_and_assert([bin_path, "x", compression_path, decompression_path])

🤖 Prompt for AI Agents

In integration-tests/tests/test_identity_transformation.py around lines 142 to 148, there is duplication with the existing clp-s helper; add a new helper named _clp_compress_and_decompress(core_config: CoreConfig, test_paths: CompressionTestConfig) that mirrors the clp-s implementation but uses core_config.clp_binary_path, sets compression_path and decompression_path from test_paths, builds the compression command list (bin_path, "c", "--progress", "--remove-path-prefix", src_path, compression_path, src_path), calls run_and_assert for compression, then calls run_and_assert to decompress ([bin_path, "x", compression_path, decompression_path]); finally update test_clp_identity_transform to call this new helper instead of duplicating the commands.

gonna do this in a future PR.

@Bill-hbrhbr That makes sense! Keeping this PR focused on the integration test config refactoring is a good approach. The helper function can definitely be added later when you have a chance to address code duplication more systematically.

🐰
╱|、
(˚ˎ 。
|、⌒)
U U

🧠 Learnings used

Learnt from: AVMatthews PR: y-scope/clp#595 File: components/core/tests/test-end_to_end.cpp:59-65 Timestamp: 2024-11-19T17:30:04.970Z Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.

Learnt from: Bill-hbrhbr PR: y-scope/clp#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: Bill-hbrhbr PR: y-scope/clp#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 PR: y-scope/clp#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.

kirkrodrigues · 2025-09-30T14:59:32Z

+    core_config: CoreConfig,
+    test_paths: CompressionTestConfig,


Hm, do you know why the formatter didn't put these on a single line? I feel like that's what black would do.

I think it's due to my extra comma.

Also, I've set up the linting process so that we don't run both black and ruff for a single directory. I think it's ruff for integration-tests

kirkrodrigues

For the PR title, how about:

fix(integration-tests): Allow CLP core binary tests to run without depending on the clp package (fixes #1281).

…pending on the clp package (fixes y-scope#1281). (y-scope#1333)

refactor integration test config so that core and package config are …

2764254

…independent from each other

Bill-hbrhbr requested a review from a team as a code owner September 25, 2025 14:59

Merge branch 'main' into fix-integration-fixtures

7f58b05

Bill-hbrhbr changed the title ~~fix(integration-tests): Refactor integration_test_config so that CLP core binaries test does not depend on a complete package (fixes #1281).~~ fix(integration-tests): Allow CLP core binary tests to run without full package dependency (fixes #1281) Sep 25, 2025

Bill-hbrhbr changed the title ~~fix(integration-tests): Allow CLP core binary tests to run without full package dependency (fixes #1281)~~ fix(integration-tests): Allow CLP core binary tests to run without full package dependency (fixes #1281). Sep 25, 2025

coderabbitai Bot reviewed Sep 25, 2025

View reviewed changes

Bill-hbrhbr linked an issue Sep 25, 2025 that may be closed by this pull request

Optimize integration test dependencies: core tests should only require core binaries #1281

Closed

Bill-hbrhbr requested a review from kirkrodrigues September 25, 2025 15:15

Bill-hbrhbr added 2 commits September 27, 2025 01:10

Merge branch 'main' into fix-integration-fixtures

247c048

Merge branch 'main' into fix-integration-fixtures

0802644

kirkrodrigues reviewed Sep 30, 2025

View reviewed changes

Bill-hbrhbr added 2 commits October 1, 2025 04:31

Merge branch 'main' into fix-integration-fixtures

f05788b

Minor lint fix

31fffc5

Bill-hbrhbr requested a review from kirkrodrigues October 1, 2025 17:40

Merge branch 'main' into fix-integration-fixtures

9b66ede

kirkrodrigues approved these changes Oct 2, 2025

View reviewed changes

Merge branch 'main' into fix-integration-fixtures

a2ca257

Bill-hbrhbr changed the title ~~fix(integration-tests): Allow CLP core binary tests to run without full package dependency (fixes #1281).~~ fix(integration-tests): Allow CLP core binary tests to run without depending on the clp package (fixes #1281). Oct 2, 2025

Bill-hbrhbr merged commit 92ef5e1 into y-scope:main Oct 2, 2025
30 of 31 checks passed

Bill-hbrhbr deleted the fix-integration-fixtures branch October 2, 2025 17:16

junhaoliao pushed a commit to junhaoliao/clp that referenced this pull request May 17, 2026

fix(integration-tests): Allow CLP core binary tests to run without de…

cb18891

…pending on the clp package (fixes y-scope#1281). (y-scope#1333)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(integration-tests): Allow CLP core binary tests to run without depending on the clp package (fixes #1281).#1333

fix(integration-tests): Allow CLP core binary tests to run without depending on the clp package (fixes #1281).#1333
Bill-hbrhbr merged 8 commits into
y-scope:mainfrom
Bill-hbrhbr:fix-integration-fixtures

Bill-hbrhbr commented Sep 25, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 25, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Sep 25, 2025 •

edited

Loading

Uh oh!

Bill-hbrhbr Sep 25, 2025

Uh oh!

coderabbitai Bot Sep 25, 2025

Uh oh!

kirkrodrigues Sep 30, 2025

Uh oh!

Bill-hbrhbr Oct 1, 2025

Uh oh!

Bill-hbrhbr Oct 1, 2025

Uh oh!

kirkrodrigues left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Description

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related issues

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bill-hbrhbr Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

kirkrodrigues Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Bill-hbrhbr Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Bill-hbrhbr Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kirkrodrigues left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bill-hbrhbr commented Sep 25, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 25, 2025 •

edited

Loading

coderabbitai Bot Sep 25, 2025 •

edited

Loading