ci(integration-tests): Add workflow for core integration tests. - #2334
ci(integration-tests): Add workflow for core integration tests.#2334quinntaylormitchell wants to merge 42 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe CLP workflow now recognizes integration-test changes, packages additional binaries, and runs Ubuntu Jammy core integration tests after a successful binary build, uploading test logs regardless of test outcome. ChangesCLP Integration-Test Workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant CLPArtifact
participant IntegrationTests
participant TestLogs
GitHubActions->>CLPArtifact: Download and untar Ubuntu Jammy binaries
GitHubActions->>IntegrationTests: Install uv and MariaDB Connector/C headers
IntegrationTests->>IntegrationTests: Run pytest -m 'smoke or core'
GitHubActions->>TestLogs: Upload integration-test logs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/clp-integration-tests.yaml:
- Line 6: The path-ignore pattern `.github/*` uses a single wildcard which only
matches immediate children of the `.github/` directory and will not match nested
subdirectories like `.github/workflows/`. To ensure consistency with the
recursive matching pattern (`**`) used in lines 7-8 and to properly ignore all
changes under the entire `.github/` directory, update the pattern from
`.github/*` to `.github/**`.
- Around line 29-31: The actions/checkout step in the workflow defaults to
persist-credentials: true, which stores the GitHub token in the git config and
could expose it if untrusted code is executed during the task tests:integration
command. Add persist-credentials: false to the with section of the
actions/checkout action to disable credential persistence, unless subsequent
workflow steps explicitly require authenticated git operations.
- Around line 23-43: The workflow currently lacks an explicit permissions block,
which means it inherits default GitHub Actions permissions that may be broader
than necessary. Add a top-level permissions block after the concurrency block
(if present) to define minimal explicit permissions required for the integration
tests workflow. Set all default permissions to false and only enable specific
permissions that the integration tests genuinely require (such as contents: read
for checking out code). If the integration tests need additional access like
pulling from GitHub Container Registry, add packages: read accordingly. This
follows the principle of least privilege and reduces security risk if the
workflow or its dependencies are compromised.
- Line 24: Add a descriptive `name` field to the `integration-tests` job
definition in the workflow file. After the job ID `integration-tests:`, add a
`name` property with a clear, human-readable description of what the job does.
This improves visibility and readability in the GitHub Actions UI when viewing
workflow runs.
- Around line 37-39: The uv installation action in the install-uv step specifies
the version as "0.8" in shorthand format, but the uv package manager requires
the complete semantic versioning format with the patch version included. Change
the version parameter value from "0.8" to "0.8.0" to match the canonical semver
format that uv uses for official releases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c447b93e-4b15-4cc2-93bc-6464d321aa04
📒 Files selected for processing (1)
.github/workflows/clp-integration-tests.yaml
quinntaylormitchell
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/clp-artifact-build.yaml:
- Around line 692-694: Update the actions/checkout step in the artifact build
job to set persist-credentials to false alongside the existing recursive
submodules option, leaving the pinned checkout action and other settings
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4eebba9-eb52-499f-9914-117063b1bdc0
📒 Files selected for processing (1)
.github/workflows/clp-artifact-build.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/clp-artifact-build.yaml (2)
734-769: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not run pull-request-controlled integration tests on the shared self-hosted runner.
This
y-scopeworkflow is triggered bypull_requestand has nopull_request_targetguard.ubuntu-jammy-integration-tests-coreroutes through*runner(self-hosted,x64,ubuntu-noble), checks out PR code, unpacks PR-produced binaries, and runsuv run pytest -m 'smoke or core'; a malicious PR can execute arbitrary code on that shared runner. Use an isolated hosted or ephemeral runner for PR runs, or run self-hosted jobs only for trusted pushes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/clp-artifact-build.yaml around lines 734 - 769, Update the ubuntu-jammy-integration-tests-core job’s runs-on configuration to prevent pull_request-triggered, PR-controlled integration tests from executing on the shared self-hosted *runner. Use an isolated hosted or ephemeral runner for PR runs, or gate the self-hosted path so it runs only for trusted push events; preserve the existing test steps and environment.
734-734: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRun this integration-test job on the Ubuntu Jammy host it’s meant to validate.
*runnerroutesy-scoperuns to["self-hosted", "x64", "ubuntu-noble"]and other runs toubuntu-24.04, so these core integration tests can exercise Jammy-built binaries on a later host OS. Use an explicit Jammy runner (or containerised Jammy host) for this job, or rename it if running Jammy artifacts on Noble/24.04 is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/clp-artifact-build.yaml at line 734, Update the integration-test job’s runs-on setting from the shared runner alias to an explicit Ubuntu Jammy runner or Jammy container host. Ensure this job validates Jammy-built binaries on Jammy, and only rename or retain the existing configuration if running them on Noble/24.04 is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/clp-artifact-build.yaml:
- Around line 734-769: Update the ubuntu-jammy-integration-tests-core job’s
runs-on configuration to prevent pull_request-triggered, PR-controlled
integration tests from executing on the shared self-hosted *runner. Use an
isolated hosted or ephemeral runner for PR runs, or gate the self-hosted path so
it runs only for trusted push events; preserve the existing test steps and
environment.
- Line 734: Update the integration-test job’s runs-on setting from the shared
runner alias to an explicit Ubuntu Jammy runner or Jammy container host. Ensure
this job validates Jammy-built binaries on Jammy, and only rename or retain the
existing configuration if running them on Noble/24.04 is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a77e71c-fc0f-42c7-805a-31b75d432379
📒 Files selected for processing (1)
.github/workflows/clp-artifact-build.yaml
| - "components/core/src/**" | ||
| - "components/core/tests/**" | ||
| - "components/core/tools/scripts/utils/build-and-run-unit-tests.py" | ||
| - "integration-tests/**" |
There was a problem hiding this comment.
Can we separate this into its own filter? Otherwise, all core binary jobs will get triggered anytime the integration tests change. I guess you want ubuntu-jammy-binaries to be triggered when the integration-tests change, so perhaps change that job to also be triggered when the new integration-tests filter is set.
| )}} | ||
|
|
||
| ubuntu-jammy-integration-tests-core: | ||
| name: "ubuntu-jammy-integration-tests-core" |
There was a problem hiding this comment.
Do we need the name for anything?
There was a problem hiding this comment.
I don't think so; I was following suit from ubuntu-jammy-lint. Should I keep it for consistency or remove it?
There was a problem hiding this comment.
I think you can remove it. I think the one for ubuntu-jammy-lint was so that we could make ubuntu-jammy-lint a required workflow (for the PR to be merged); but there were issues with doing that and I guess we never removed the name.
There was a problem hiding this comment.
Gotcha, removed it
| # Run if the ancestor jobs were successful/skipped and building clp was successful. | ||
| if: "!cancelled() && !failure() && needs.ubuntu-jammy-binaries.result == 'success'" |
There was a problem hiding this comment.
| # Run if the ancestor jobs were successful/skipped and building clp was successful. | |
| if: "!cancelled() && !failure() && needs.ubuntu-jammy-binaries.result == 'success'" |
I don't think this is necessary since needs: "ubuntu-jammy-binaries" implies the same.
| submodules: "recursive" | ||
| persist-credentials: false |
There was a problem hiding this comment.
| submodules: "recursive" | |
| persist-credentials: false | |
| persist-credentials: false | |
| submodules: "recursive" |
Nit: Alphbetize
| tar xf clp.tar | ||
| rm clp.tar | ||
|
|
||
| - name: "Install uv" |
There was a problem hiding this comment.
Let's use install-uv from yscope-dev-utils instead.
| CLP_BUILD_DIR: "../${{env.BUILD_DIR}}" | ||
| CLP_CORE_BINS_DIR: >- | ||
| ../${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}} | ||
| HOME: "/tmp" |
There was a problem hiding this comment.
Can we add a comment for why this is necessary?
There was a problem hiding this comment.
Sorry, I still don't understand why we need to set HOME?
There was a problem hiding this comment.
Oops we don't, sorry. That's v old
| CLP_CORE_BINS_DIR: >- | ||
| ../${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}} | ||
| HOME: "/tmp" | ||
| run: "uv run pytest -m 'smoke or core'" |
There was a problem hiding this comment.
Why do we need to run the smoke tests for CLP-core?
There was a problem hiding this comment.
I had it there to provide an indication that pytest was working correctly, and that any other test failures in that run were due to some problem with how I had set up other aspects of the workflow. But we don't need it though, cause the core tests don't import anything from the CLP code. Will remove.
There was a problem hiding this comment.
Gotcha. Yeah, we can add them in the package integration tests PR.
|
|
||
| - name: "Upload the core integration test logs" | ||
| if: "always()" | ||
| uses: "actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f" # v6.0.0 |
There was a problem hiding this comment.
Use the version used elsewhere in the workflow
There was a problem hiding this comment.
This file uses version ending with 789f once in this workflow (not counting this new usage), and uses version ending with fa02 once as well. Which one should I use?
There was a problem hiding this comment.
Oh whoops, let's keep this and fix the other one in another PR.
|
|
||
| 1. building (Linux) container images containing CLP-core's dependencies, | ||
| 2. building CLP-core and running its unit tests, and | ||
| 2. building CLP-core and running its unit & integration tests, and |
There was a problem hiding this comment.
| 2. building CLP-core and running its unit & integration tests, and | |
| 2. building CLP-core and running its unit & integration tests, and |
| * `ubuntu-jammy-binaries-image`: Builds an Ubuntu Jammy container image containing CLP-core's | ||
| binaries built in the `ubuntu-jammy-binaries` job. | ||
| * `ubuntu-jammy-integration-tests-core`: Runs CLP-core's integration tests using the binaries built | ||
| in the `ubuntu-jammy-binaries` job. The test logs are uploaded at the end of the test run. |
There was a problem hiding this comment.
| in the `ubuntu-jammy-binaries` job. The test logs are uploaded at the end of the test run. | |
| in the `ubuntu-jammy-binaries` job, and then uploads the logs from the tests. |
| tar xf clp.tar | ||
| rm clp.tar | ||
|
|
||
| - uses: "./tools/yscope-dev-utils/exports/github/actions/install-uv" |
There was a problem hiding this comment.
| - uses: "./tools/yscope-dev-utils/exports/github/actions/install-uv" | |
| - uses: "./tools/yscope-dev-utils/exports/github/actions/install-uv" | |
| with: | |
| version: "0.8" |
It seems like all other installations of uv use 0.8.
| CLP_BUILD_DIR: "../${{env.BUILD_DIR}}" | ||
| CLP_CORE_BINS_DIR: >- | ||
| ../${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}} | ||
| HOME: "/tmp" |
There was a problem hiding this comment.
Sorry, I still don't understand why we need to set HOME?
| CLP_CORE_BINS_DIR: >- | ||
| ../${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}} | ||
| HOME: "/tmp" | ||
| run: "uv run pytest -m 'smoke or core'" |
There was a problem hiding this comment.
Gotcha. Yeah, we can add them in the package integration tests PR.
| from job_orchestration.scheduler.constants import CompressionJobStatus | ||
|
|
||
|
|
||
| @pytest.mark.smoke |
There was a problem hiding this comment.
We can undo the changes in this file since it's no longer relevant to this PR, right?
| core: mark tests that test the CLP core binaries | ||
| package: mark tests that use the CLP package | ||
| search: mark tests that test search | ||
| smoke: mark tests that are smoke tests |
| - name: "Run the integration tests for CLP's core" | ||
| working-directory: "integration-tests" | ||
| env: | ||
| # Tell pytest where the CLP binaries are, and where it should store the test logs. |
There was a problem hiding this comment.
| # Tell pytest where the CLP binaries are, and where it should store the test logs. |
I think it's actually better to not have this comment and instead document these environment variables (if we haven't already) in another PR.
3 participants
Description
This PR adds the
ubuntu-jammy-integration-tests-coreworkflow toclp-artifact-build. It downloads the binary tarball artifact, untars the binaries, installs dependencies, then runs smoke tests and core tests withuv run pytest -m 'smoke or core'. The integration test logs are uploaded as an artifact after the run.The dev docs are updated w.r.t. the changes in
clp-artifact-build.Checklist
breaking change.
Validation performed
The
ubuntu-jammy-integration-tests-coreworkflow passes.Summary by CodeRabbit
Tests
Chores
clpbundle now includes additional executables/artifacts: indexer, log-converter, and reducer-server.