Repository navigation
ci(integration-tests): Add GitHub workflow job to run CLP-core integration tests on every push. #2334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci(integration-tests): Add GitHub workflow job to run CLP-core integration tests on every push. #2334
Changes from 39 commits
35c0813
bb52e05
1605df7
b33cd44
bb47ca8
cbc4ac3
d4ac376
de7a3d5
397e4f1
52338b0
14c8a77
aa498c1
4a4e700
8f6c8dc
205f613
426adc6
3da6391
540da75
7c5fe43
0f861d3
864dcb2
98ffe9e
0acc0b3
981e81f
bd54f23
18c78bb
96d2b9d
247ebdb
612e553
202ec31
0320b76
87ac4ec
81a617c
3481d92
523ca73
913713f
b302435
a048c19
12e48d4
a65e988
5afd24a
454b9c9
72e56a0
49c5c27
c3cd074
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -103,6 +103,7 @@ jobs: | |||||||||
| - "components/core/src/**" | ||||||||||
| - "components/core/tests/**" | ||||||||||
| - "components/core/tools/scripts/utils/build-and-run-unit-tests.py" | ||||||||||
| - "integration-tests/**" | ||||||||||
| - "taskfile.yaml" | ||||||||||
| - "taskfiles/**" | ||||||||||
| - "tools/scripts/deps-download/**" | ||||||||||
|
|
@@ -575,7 +576,8 @@ jobs: | |||||||||
|
|
||||||||||
| mkdir -p "${output_dir}" | ||||||||||
| cd "$GITHUB_WORKSPACE/components/core/build" | ||||||||||
| tar cfvv "${output_dir}/clp.tar" clg clp clp-s glt make-dictionaries-readable | ||||||||||
| tar cfvv "${output_dir}/clp.tar" \ | ||||||||||
| clg clo clp clp-s glt indexer log-converter make-dictionaries-readable reducer-server | ||||||||||
| shell: "bash" | ||||||||||
|
|
||||||||||
| - if: "matrix.upload_binaries == true" | ||||||||||
|
|
@@ -724,6 +726,57 @@ jobs: | |||||||||
| 'build/lint-clang-tidy/**/*' | ||||||||||
| )}} | ||||||||||
|
|
||||||||||
| ubuntu-jammy-integration-tests-core: | ||||||||||
| name: "ubuntu-jammy-integration-tests-core" | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the name for anything?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so; I was following suit from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can remove it. I think the one for
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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'" | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't think this is necessary since
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||||||||||
| needs: "ubuntu-jammy-binaries" | ||||||||||
| runs-on: *runner | ||||||||||
| env: | ||||||||||
| OS_NAME: "ubuntu-jammy" | ||||||||||
| BUILD_DIR: "build" | ||||||||||
| steps: | ||||||||||
| - uses: "actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd" # v6.0.2 | ||||||||||
| with: | ||||||||||
| submodules: "recursive" | ||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||||||||||
| persist-credentials: false | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: Alphbetize
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||||||||||
|
|
||||||||||
| - name: "Download the binaries artifact" | ||||||||||
| uses: "actions/download-artifact@95815c38cf2ff2164869cbab79da8d1f422bc89e" | ||||||||||
| with: | ||||||||||
| name: "${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}}" | ||||||||||
| path: "${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}}" | ||||||||||
|
|
||||||||||
| - name: "Untar binaries" | ||||||||||
| working-directory: >- | ||||||||||
| ${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}} | ||||||||||
| run: |- | ||||||||||
| tar xf clp.tar | ||||||||||
| rm clp.tar | ||||||||||
|
|
||||||||||
| - name: "Install uv" | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||||||||||
| uses: "astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990" # v8.3.2 | ||||||||||
|
|
||||||||||
| - name: "Install MariaDB Connector/C" | ||||||||||
| run: "sudo apt-get update && sudo apt-get install -y libmariadb-dev" | ||||||||||
|
|
||||||||||
| - name: "Run core integration tests" | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||||||||||
| working-directory: "integration-tests" | ||||||||||
| env: | ||||||||||
| CLP_BUILD_DIR: "../${{env.BUILD_DIR}}" | ||||||||||
| CLP_CORE_BINS_DIR: >- | ||||||||||
| ../${{env.BUILD_DIR}}/${{env.BINARIES_ARTIFACT_NAME_PREFIX}}${{env.OS_NAME}} | ||||||||||
| HOME: "/tmp" | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a comment for why this is necessary?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I still don't understand why we need to set
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops we don't, sorry. That's v old |
||||||||||
| run: "uv run pytest -m 'smoke or core'" | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to run the smoke tests for CLP-core?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the version used elsewhere in the workflow
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh whoops, let's keep this and fix the other one in another PR. |
||||||||||
| with: | ||||||||||
| name: "clp-integration-test-core-logs" | ||||||||||
| path: "${{env.BUILD_DIR}}/integration_tests/test_logs" | ||||||||||
| retention-days: 3 | ||||||||||
|
|
||||||||||
| package-image: | ||||||||||
| name: "package-image-${{matrix.arch}}" | ||||||||||
| # NOTE: The aarch64 job is skipped on PRs, so we accept both 'success' and 'skipped'. | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ builds, testing, and linting. We briefly describe each workflow below. | |||||
| This workflow is responsible for: | ||||||
|
|
||||||
| 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 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed |
||||||
| 3. building a container image containing CLP's package components. | ||||||
|
|
||||||
| To minimize build times, the jobs in the workflow are organized in the directed acyclic graph (DAG) | ||||||
|
|
@@ -47,6 +47,7 @@ flowchart LR | |||||
| ubuntu-jammy-deps-image --> ubuntu-jammy-binaries | ||||||
| ubuntu-jammy-deps-image --> package-image | ||||||
| ubuntu-jammy-binaries --> ubuntu-jammy-binaries-image | ||||||
| ubuntu-jammy-binaries --> ubuntu-jammy-integration-tests-core | ||||||
| ::: | ||||||
|
|
||||||
| Arrows between jobs indicate a dependency. The jobs are as follows: | ||||||
|
|
@@ -80,6 +81,8 @@ Arrows between jobs indicate a dependency. The jobs are as follows: | |||||
| core's unit tests. | ||||||
| * `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. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed |
||||||
|
|
||||||
| When the PR or commit doesn't change any of the files that affect CLP's dependencies (or the | ||||||
| dependency container images), then the dependency container images won't be rebuilt; instead the | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,4 +30,5 @@ markers = | |
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||
| startstop: mark tests that test start-stop | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| """Smoke tests to validate that CLP Python projects can be imported without errors.""" | ||
|
|
||
| import pytest | ||
| from clp_mcp_server.constants import QueryJobType | ||
| from clp_package_utils.general import JobType | ||
| from clp_py_utils.clp_config import StorageEngine | ||
| from job_orchestration.scheduler.constants import CompressionJobStatus | ||
|
|
||
|
|
||
| @pytest.mark.smoke | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can undo the changes in this file since it's no longer relevant to this PR, right?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||
| def test_clp_native_py_project_enum_classes() -> None: | ||
| """ | ||
| Verifies that the following CLP Python projects can be imported successfully by testing | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-binariesto be triggered when the integration-tests change, so perhaps change that job to also be triggered when the new integration-tests filter is set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed