Skip to content

Conversation

@Bill-hbrhbr
Copy link
Contributor

@Bill-hbrhbr Bill-hbrhbr commented Nov 4, 2025

  • Add tasks to run smoke tests to validate import success.
  • Ensure CLP py projects contain typing file to satisfy mypy checks.
  • Install MariaDB Connector/C in Python lint CI, which depends on the mariadb Python package.

Description

This PR ensures that CLP native Python projects— clp-py-utils, clp-package-utils, clp-mcp-server, and job-orchestration—are properly installed and discoverable within the integration test environment.

These libraries already define dataclasses, structures, and utilities that can be directly leveraged in integration tests, allowing us to reuse existing logic and avoid reinventing the wheel when building end-to-end tests for the full CLP package.

This PR verifies installation success by testing enum classes (e.g. StorageEngine in clp_py_utils.clp_config) can be imported and constructed successfully.

In addition, this PR introduces an empty py.typed marker file to declare typing support for aforementioned CLP native python packages, enabling type checkers like MyPy to recognize the package as typed.

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

  • Newly added integration tests pass.

Summary by CodeRabbit

  • Tests

    • Added validation to ensure Python project dependencies import without errors.
  • Chores

    • Updated CI/CD pipeline to install MariaDB Connector/C on supported platforms.
    • Enhanced integration test infrastructure with new dependency configuration.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

This pull request adds MariaDB Connector/C installation to the GitHub Actions lint workflow, configures new dependency groups and uv tool settings in the integration tests project, introduces a smoke test validating CLP Python project imports, and adds a corresponding integration test task to the taskfile.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
.github/workflows/clp-lint.yaml
Adds a new "Install MariaDB Connector/C" step in the lint-check job. The step handles installation for both ubuntu-24.04 and macos-15, sets MARIADB_CONFIG_PATH, and exports it via GITHUB_ENV. Includes fallback error handling for unexpected OS values.
Project Configuration
integration-tests/pyproject.toml
Adds a new clp dependency group containing clp-mcp-server, clp-package-utils, clp-py-utils, and job-orchestration. Introduces [tool.uv] configuration with default-groups set to ["clp", "dev"] and defines local paths for the four clp-related dependencies.
Integration Test
integration-tests/tests/test_clp_native_py_project_imports.py
New smoke test file that validates CLP Python project imports by asserting enum conversions for QueryJobType, JobType, StorageEngine, and CompressionJobStatus against representative values.
Test Task Definition
taskfiles/tests/integration.yaml
Adds new clp-py-project-imports task that runs pytest on the new imports test file. Updates default task dependencies to include the new task before the core task. Modifies core task command from uv run python -m pytest -m core to uv run pytest -m core.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Workflow step addition is straightforward; verify OS detection logic and MariaDB paths are correct for both platforms
  • Dependency group and uv tool configuration follow standard patterns; confirm all local paths and dependency names are accurate
  • Integration test is a simple smoke test with clear assertions; verify enum values align with actual class definitions

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective: installing CLP Python projects to enable their use in integration tests, with mention of fixing linting issues.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • 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.

@Bill-hbrhbr Bill-hbrhbr changed the title feat(integration-tests): Add clp-py-utils import. feat(integration-tests): Install CLP native Python projects and ensure they are typed to satisfy python linters. Nov 5, 2025
@Bill-hbrhbr Bill-hbrhbr marked this pull request as ready for review November 5, 2025 18:22
@Bill-hbrhbr Bill-hbrhbr requested a review from a team as a code owner November 5, 2025 18:22
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec509bf and fdcf80e.

⛔ Files ignored due to path filters (1)
  • integration-tests/uv.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • .github/workflows/clp-lint.yaml (1 hunks)
  • components/clp-mcp-server/pyproject.toml (1 hunks)
  • components/clp-package-utils/pyproject.toml (1 hunks)
  • components/clp-py-utils/pyproject.toml (1 hunks)
  • components/job-orchestration/pyproject.toml (1 hunks)
  • integration-tests/pyproject.toml (2 hunks)
  • integration-tests/tests/test_clp_native_py_project_imports.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: 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/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/pyproject.toml
  • components/clp-py-utils/pyproject.toml
  • integration-tests/tests/test_clp_native_py_project_imports.py
  • components/clp-package-utils/pyproject.toml
📚 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
  • .github/workflows/clp-lint.yaml
📚 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-07-23T09:54:45.185Z
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.

Applied to files:

  • .github/workflows/clp-lint.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.

Applied to files:

  • .github/workflows/clp-lint.yaml
📚 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:

  • .github/workflows/clp-lint.yaml
📚 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:

  • .github/workflows/clp-lint.yaml
📚 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/test_clp_native_py_project_imports.py
🧬 Code graph analysis (1)
integration-tests/tests/test_clp_native_py_project_imports.py (2)
components/clp-package-utils/clp_package_utils/general.py (1)
  • JobType (102-106)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
  • StorageEngine (96-98)
⏰ 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 (8)
components/clp-mcp-server/pyproject.toml (1)

34-36: LGTM!

The wheel build target configuration correctly packages the clp_mcp_server module with typing support.

components/clp-py-utils/pyproject.toml (1)

25-28: LGTM!

The wheel build target configuration correctly packages the clp_py_utils module with typing support.

components/clp-package-utils/pyproject.toml (1)

20-23: LGTM!

The wheel build target configuration correctly packages the clp_package_utils module with typing support.

integration-tests/pyproject.toml (2)

19-24: LGTM!

The new "clp" dependency group correctly lists all four CLP native Python packages, enabling integration tests to import and use them.


79-86: LGTM!

The uv tool configuration correctly sets up path-based dependencies for local development, pointing to the component directories. This enables integration tests to import from local development versions of the CLP packages.

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

1-10: LGTM!

The imports correctly reference enums from all four CLP native Python packages, serving as a smoke test to verify package installation and discoverability.


12-24: LGTM!

All enum values have been verified and are correct:

  • QueryJobType.SEARCH_OR_AGGREGATION = 0
  • CompressionJobStatus.PENDING = 0
  • JobType.COMPRESSION uses KebabCaseStrEnum with auto(), which converts to "compression"
  • StorageEngine.CLP uses KebabCaseStrEnum with auto(), which converts to "clp"

The test correctly verifies that all four CLP native Python packages import successfully and their enum classes function as expected.

components/job-orchestration/pyproject.toml (1)

26-29: Approved: Wheel build configuration and PEP 561 typing support correctly implemented.

All py.typed marker files are confirmed to exist across all CLP native Python packages. The wheel build target configuration in pyproject.toml is correct and follows Hatch packaging conventions.

@Bill-hbrhbr Bill-hbrhbr changed the title feat(integration-tests): Install CLP native Python projects and ensure they are typed to satisfy python linters. feat(integration-tests): Install CLP native Python projects to enable their use in future integration tests: Nov 5, 2025
Copy link
Collaborator

@quinntaylormitchell quinntaylormitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than the high-level stuff we talked about offline, I ran task tests:integration and test_clp_native_py_project_enum_classes() didn't run (I made sure by adding some log statements to it, which never printed). the projects are importing correctly though, cause I tried using StorageEngine.CLP in another file (just to check) and it worked fine. maybe we should remove test_clp_native_py_project_enum_classes() like we talked about. left a comment on it just in case we don't.

Copy link
Collaborator

@quinntaylormitchell quinntaylormitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one little comment

Co-authored-by: Quinn Taylor Mitchell <[email protected]>
quinntaylormitchell added a commit to quinntaylormitchell/clp that referenced this pull request Nov 5, 2025
Copy link
Collaborator

@quinntaylormitchell quinntaylormitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ab7b791 and c9986d4.

📒 Files selected for processing (1)
  • taskfiles/tests/integration.yaml (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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: 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: 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
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: 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-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:

  • taskfiles/tests/integration.yaml
⏰ 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). (18)
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: musllinux_1_2-x86_64-static-linked-bins
  • GitHub Check: ubuntu-jammy-lint
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: package-image
  • GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-static-linked-bins
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
  • GitHub Check: rust-checks (macos-15)
  • GitHub Check: rust-checks (ubuntu-22.04)
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: lint-check (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: build-macos (macos-15, true)
  • GitHub Check: build-macos (macos-15, false)
  • GitHub Check: build-macos (macos-14, false)

Comment on lines 34 to 37
[tool.hatch.build.targets.wheel]
packages = ["clp_mcp_server"]
include = ["clp_mcp_server/py.typed"]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, why do we explicitly need these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without include = ["clp_mcp_server/py.typed"], mypy will complain missing typing info when we use import clp_mcp_server. We ship the py.typed with the install wheel.
For packages = ["clp_mcp_server"], since we now specify [tool.hatch.build.targets.wheel], it becomes a required field, otherwise integration tests would complain that it cannot find clp_mcp_server package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I have no idea, but now after removing these lines mypy doesn't complain anymore

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
taskfiles/tests/integration.yaml (1)

29-31: Align pytest invocation with established patterns in the file.

The clp-py-project-imports task uses uv run pytest while the cache-clear task (line 16) consistently uses uv run python -m pytest. For clarity and consistency across the integration test tasks, please align this task with the established pattern.

Apply this diff to align with the established pattern:

  clp-py-project-imports:
    dir: "{{.G_INTEGRATION_TESTS_DIR}}"
-   cmd: "uv run pytest tests/test_clp_native_py_project_imports.py"
+   cmd: "uv run python -m pytest tests/test_clp_native_py_project_imports.py"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ff0499 and 5729abc.

📒 Files selected for processing (1)
  • taskfiles/tests/integration.yaml (2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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: 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: 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
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).
📚 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:

  • taskfiles/tests/integration.yaml
📚 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:

  • taskfiles/tests/integration.yaml
📚 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:

  • taskfiles/tests/integration.yaml
📚 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:

  • taskfiles/tests/integration.yaml
📚 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:

  • taskfiles/tests/integration.yaml
📚 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:

  • taskfiles/tests/integration.yaml
📚 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:

  • taskfiles/tests/integration.yaml

quinntaylormitchell added a commit to quinntaylormitchell/clp that referenced this pull request Nov 7, 2025
@Bill-hbrhbr
Copy link
Contributor Author

@coderabbitai

make an issue to switch all python -m pytest to pytest

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 7, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@Bill-hbrhbr I'll search for all occurrences of python -m pytest in the codebase and create an issue for you.


🏁 Script executed:

#!/bin/bash
# Find all occurrences of 'python -m pytest' in the codebase
echo "=== Searching for 'python -m pytest' patterns ==="
rg -n 'python -m pytest' -g '!*.md' -g '!CHANGELOG*'

Length of output: 374


🏁 Script executed:

#!/bin/bash
# Create GitHub issue to switch all python -m pytest to pytest
gh issue create \
  --title "Switch all \`python -m pytest\` invocations to \`pytest\`" \
  --body "## Context

This PR made a partial change to use \`pytest\` directly instead of \`python -m pytest\` in the integration test tasks. However, there are still remaining occurrences that should be updated for consistency.

## Remaining Occurrences

All occurrences are in \`taskfiles/tests/integration.yaml\`:
- Line 16: \`uv run python -m pytest --cache-clear --collect-only --override-ini addopts=\"\" --quiet\`
- Line 31: \`uv run python -m pytest tests/test_clp_native_py_project_imports.py\`

## Goal

Standardise all pytest invocations across the integration test tasks to use \`pytest\` directly rather than \`python -m pytest\`.

## References

- Related PR: https://github.com/y-scope/clp/pull/1549
- Comment: https://github.com/y-scope/clp/pull/1549#discussion_r1868820000
- Requested by: @Bill-hbrhbr" \
  --assignee Bill-hbrhbr \
  --repo y-scope/clp

Length of output: 42


@Bill-hbrhbr I've created issue #1571 to track switching all python -m pytest invocations to pytest.

I found 3 occurrences in taskfiles/tests/integration.yaml. This PR already changed line 27 (the core task) but there are 2 remaining:

  • Line 16: cache-clear command
  • Line 31: the newly added clp-py-project-imports task

The issue has been assigned to you with references back to this PR and comment.


🧠 Learnings used
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: 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: 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
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).

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the PR title & body, how about:

feat(integration-tests): Install CLP Python projects to enable their use in integration tests and fix new linting issues:
* Add integration test to validate importing symbols from the projects.
* Add a `py.typed` file to all uv projects so that they can be imported into projects that use `mypy`.
* Install the native MariaDB Connector/C package which is now required in the `clp-lint` workflow.

@Bill-hbrhbr Bill-hbrhbr changed the title feat(integration-tests): Install CLP native Python projects to enable their use in future integration tests: feat(integration-tests): Install CLP Python projects to enable their use in integration tests and fix new linting issues: Nov 7, 2025
@Bill-hbrhbr Bill-hbrhbr merged commit f7a978c into y-scope:main Nov 7, 2025
33 checks passed
@Bill-hbrhbr Bill-hbrhbr deleted the integration-tests-import-local-python-libs branch November 7, 2025 16:42
quinntaylormitchell added a commit to quinntaylormitchell/clp that referenced this pull request Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants