ci: Add tasks to run unit tests and GitHub workflow to run non-storage unit tests. - #30
Conversation
WalkthroughThis pull request introduces several changes to the project's configuration files, primarily focused on enhancing the CI/CD pipeline and task management. A new GitHub Actions workflow for running unit tests is added, along with structured configurations for build and test tasks. The linting process for YAML files is expanded to include additional files, and references to new task files are integrated into existing configurations. These changes collectively aim to streamline the build and testing processes within the project. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
test-tasks.yaml (2)
12-18: Consider parallelizing build stepsThe sequential build steps could be optimized if there are no interdependencies between targets.
Consider using task dependencies instead of sequential commands if the targets can be built independently:
unit-test: + deps: + - "build:target-unit-test" + - "build:target-executor" + - "build:target-worker" cmds: - - task: "build:target" - vars: {TARGET: "unitTest"} - - task: "build:target" - vars: {TARGET: "spider_task_executor"} - - task: "build:target" - vars: {TARGET: "worker_test"} - "{{.G_TEST_BINARY}} \"~[storage]\""
19-20: Document storage test exclusion rationaleThe comment about skipping storage tests should be more descriptive for future maintainers.
- # Tests tagged with storage requires mysql. Skip them for now. + # TODO: Storage-tagged tests require MySQL database setup. + # These tests are temporarily excluded from CI to avoid external dependencies. + # Consider adding MySQL service container in GitHub Actions for complete testing. - "{{.G_TEST_BINARY}} \"~[storage]\""build-tasks.yaml (1)
19-20: Consider adding build output controlThe parallel build command could benefit from configurable job count and output verbosity.
- - "cmake --build {{.G_BUILD_SPIDER_DIR}} --target {{.TARGET}} --parallel" + - "cmake --build {{.G_BUILD_SPIDER_DIR}} --target {{.TARGET}} --parallel {{.JOBS | default \"$(nproc)\"}} {{if eq .VERBOSE \"true\"}}--verbose{{end}}".github/workflows/unit-tests.yaml (2)
1-10: LGTM! Consider documenting timezone impact.The workflow triggers are well-configured. The scheduling at 00:15 UTC is thoughtfully chosen to avoid high-load periods.
Consider adding a comment about the timezone's impact on different regions, as 00:15 UTC might be during working hours in some locations.
19-33: Consider expanding the test matrix.While using Ubuntu is a good start, consider adding more operating systems to catch platform-specific issues early.
strategy: matrix: - os: ["ubuntu-latest"] + os: ["ubuntu-latest", "macos-latest", "windows-latest"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/unit-tests.yaml(1 hunks)build-tasks.yaml(1 hunks)lint-tasks.yaml(1 hunks)taskfile.yaml(1 hunks)test-tasks.yaml(1 hunks)
🔇 Additional comments (6)
test-tasks.yaml (1)
6-7: Verify G_BUILD_SPIDER_DIR variable availability
The G_TEST_BINARY path depends on G_BUILD_SPIDER_DIR. Let's verify this variable is properly defined.
✅ Verification successful
G_BUILD_SPIDER_DIR variable is properly defined and used
The variable G_BUILD_SPIDER_DIR is correctly defined in taskfile.yaml as {{.G_BUILD_DIR}}/spider, where G_BUILD_DIR is also properly defined as {{.ROOT_DIR}}/build. The variable is consistently used across the build system configuration files, including the test binary path definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if G_BUILD_SPIDER_DIR is defined in other task files
rg -l "G_BUILD_SPIDER_DIR" --type yaml
Length of output: 85
Script:
#!/bin/bash
# Check the definition and usage of G_BUILD_SPIDER_DIR in the found files
rg "G_BUILD_SPIDER_DIR" --type yaml -C 2
Length of output: 1240
build-tasks.yaml (1)
10-17: LGTM! Comprehensive target enumeration
The target list properly includes all necessary components for unit testing.
taskfile.yaml (1)
6-6: LGTM! Proper test tasks integration
The test tasks inclusion is correctly positioned and maintains consistent structure with other includes.
.github/workflows/unit-tests.yaml (2)
11-18: LGTM! Security-conscious configuration.
The empty permissions block follows the principle of least privilege, and the concurrency settings efficiently manage resources by cancelling redundant runs.
48-53: Verify timeout duration and test exclusions.
The 10-minute timeout might be insufficient for complex dependency trees. Also, ensure that the test:unit-test task properly excludes MySQL-dependent tests as mentioned in the PR objectives.
✅ Verification successful
10-minute timeout is sufficient, and MySQL tests are properly excluded
The verification shows that:
- The dependency installation task (
deps:lib_install) only runs platform-specific shell scripts, which typically complete quickly - The unit test configuration explicitly excludes MySQL-dependent tests with the pattern
~[storage] - The test execution is properly configured to skip storage-tagged tests that require MySQL
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test task configuration for MySQL test exclusions
# Check test task configuration
fd -t f -e yaml "test.*\.ya?ml" | xargs cat | grep -A 5 "unit-test:"
# Look for MySQL-related test files
fd -t f "test.*\.py" | xargs grep -l "mysql"
Length of output: 278
Script:
#!/bin/bash
# Check Taskfile.yml for test configurations
cat Taskfile.yml | grep -A 10 "test:unit-test:"
# Look for test configuration files
fd -t f "Taskfile.ya?ml"
# Check for MySQL-related test files with more generic pattern
rg -l "mysql|MySQL" -g "test_*.py"
Length of output: 305
Script:
#!/bin/bash
# Look for task definition files in any location
fd -t f "task.*\.(yaml|yml)"
# Search for MySQL-related test files without filename pattern restriction
rg -l "mysql|MySQL" -t py
# Check the workflow file directly for task definitions
cat .github/workflows/unit-tests.yaml
Length of output: 1474
Script:
#!/bin/bash
# Check test task definitions
cat test-tasks.yaml
# Check dependency installation task
cat dep-tasks.yaml
# Look for Python test files
fd -t f -e py -e pytest "test"
Length of output: 1030
lint-tasks.yaml (1)
120-121: LGTM! Verify file existence.
The addition of test-tasks.yaml and build-tasks.yaml to the linting scope is appropriate. Let's verify these files exist in the repository.
✅ Verification successful
Files confirmed: YAML linting scope expansion is valid
The files test-tasks.yaml and build-tasks.yaml exist in the repository, confirming that the addition of these files to the YAML linting scope is appropriate and functional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify existence of newly added YAML files
# Check if files exist
for file in test-tasks.yaml build-tasks.yaml; do
if [ ! -f "$file" ]; then
echo "Missing file: $file"
exit 1
fi
done
Length of output: 160
Script:
#!/bin/bash
# Let's try using fd to find these YAML files as they might be in a different directory
fd -e yaml -e yml "test-tasks|build-tasks"
Length of output: 77
| - if: "matrix.os == 'macos-latest'" | ||
| name: "Install coreutils (for md5sum)" | ||
| run: "brew install coreutils" | ||
|
|
There was a problem hiding this comment.
Remove or enable macOS-specific step.
There's a macOS-specific step for installing coreutils, but macOS isn't included in the OS matrix. Either remove this step or add macOS to the matrix as suggested above.
| - if: "matrix.os == 'macos-latest'" | ||
| name: "Install coreutils (for md5sum)" | ||
| run: "brew install coreutils" | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test-tasks.yaml (2)
3-4: Consider making the test binary path more configurableThe test binary path is hardcoded to a specific location. Consider making this more flexible by allowing override through environment variables.
vars: - G_TEST_BINARY: "{{.G_BUILD_SPIDER_DIR}}/tests/unitTest" + G_TEST_BINARY: '{{default "tests/unitTest" .TEST_BINARY_PATH | printf "%s/%s" .G_BUILD_SPIDER_DIR}}'
7-11: Test organization looks good, but documentation is missingThe test tasks are well-organized into non-storage, storage, and all categories as requested in previous reviews. However, we should document these tasks.
Consider adding task descriptions:
tasks: non-storage-unit-tests: + desc: "Run all unit tests except storage-related ones" deps: - "build-unit-test" cmds: - "{{.G_TEST_BINARY}} \"~[storage]\"" storage-unit-tests: + desc: "Run only storage-related unit tests (requires MySQL setup)" deps: - "build-unit-test" cmds: - "{{.G_TEST_BINARY}} \"[storage]\"" all: + desc: "Run all unit tests including storage tests (requires MySQL setup)" deps: - "build-unit-test" cmds: - "{{.G_TEST_BINARY}}"Also applies to: 13-17, 19-23
.github/workflows/code-linting-checks.yaml (1)
Line range hint
21-47: Consider adding timeout for the linting stepWhile the dependency installation has a timeout, the actual linting step doesn't. This could potentially lead to hung jobs.
- name: "Install project dependencies " timeout-minutes: 10 run: "task deps:lib_install" - - run: "task lint:check" + - name: "Run linting checks" + timeout-minutes: 5 + run: "task lint:check"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/code-linting-checks.yaml(1 hunks).github/workflows/unit-tests.yaml(1 hunks)build-tasks.yaml(1 hunks)lint-tasks.yaml(1 hunks)taskfile.yaml(1 hunks)test-tasks.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- taskfile.yaml
- build-tasks.yaml
- lint-tasks.yaml
- .github/workflows/unit-tests.yaml
🔇 Additional comments (2)
test-tasks.yaml (1)
25-29: Verify build dependencies completeness
The build task looks well-structured, but let's verify if all required targets are included.
✅ Verification successful
Build dependencies are correctly specified
Based on the CMake configuration files, the build task's dependencies are complete and accurate. The specified targets in the YAML file ("spider_task_executor", "unitTest", "worker_test") align perfectly with the test-related targets found in the CMake configuration:
- "unitTest" is the main test executable
- "worker_test" is a shared library for worker testing
- "spider_task_executor" appears to be a core component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any additional test-related targets in CMakeLists.txt files
fd -e txt -e cmake CMakeLists | xargs rg -i "add_test|enable_testing"
Length of output: 71
Script:
#!/bin/bash
# Let's try a different approach to find test-related targets
# First, let's look for test-related targets in CMake files
fd CMakeLists.txt | xargs rg -i "test" -A 2
# Also search for any test-related targets in build configuration
fd -e yaml -e yml | xargs rg -i "test.*target"
Length of output: 1733
.github/workflows/code-linting-checks.yaml (1)
21-21: Workflow simplification looks good
The change to use only ubuntu-latest is a good simplification, as it reduces complexity while maintaining functionality. This aligns well with the PR's objective of streamlining the CI process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
docs/testing.md (3)
46-47: Add taskfile location reference.Consider adding the location of the taskfile (e.g.,
./taskfile.yaml) to help developers locate the configuration more easily.
48-53: Improve sentence variety in the documentation.Consider rewording to avoid repetitive sentence beginnings:
-* `test:non-storage-unit-tests` runs all unit tests without `[storage]` tag, i.e. do not need a storage to run. -* `test:storage-unit-tests` runs all unit tests that has `[storage]` tag. -* `test:all` runs all unit tests. - -All unit-test related tasks build `spider_task_executor`, `unitTest` and `worker_test`, which are necessary to run unit -tests. +* `test:non-storage-unit-tests`: Executes all unit tests without the `[storage]` tag (no storage required) +* `test:storage-unit-tests`: Handles all unit tests tagged with `[storage]` +* `test:all`: Executes the complete test suite + +These tasks automatically build the required components: `spider_task_executor`, `unitTest`, and `worker_test`.🧰 Tools
🪛 LanguageTool
[style] ~50-~50: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... that has[storage]tag. *test:allruns all unit tests. All unit-test related ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
57-58: Enhance workflow documentation with additional details.Consider expanding this section with:
- Specific trigger conditions (which branches/events trigger the workflow)
- The exact schedule timing for daily runs
- Future plans or requirements for incorporating storage tests
Example addition:
+The workflow triggers on: +- Push events to main branch +- Pull requests targeting main branch +- Daily at [specific time] UTC + +Note: Integration of storage-dependent tests is planned for future iterations, pending containerized MySQL setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/testing.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/testing.md
[style] ~50-~50: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... that has [storage] tag. * test:all runs all unit tests. All unit-test related ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (1)
docs/testing.md (1)
42-42: No issues with the code block closure.
| ## Test tasks | ||
|
|
||
| Three unit-test related tasks are added to taskfile. | ||
|
|
||
| * `test:non-storage-unit-tests` runs all unit tests without `[storage]` tag, i.e. do not need a storage to run. | ||
| * `test:storage-unit-tests` runs all unit tests that has `[storage]` tag. | ||
| * `test:all` runs all unit tests. | ||
|
|
||
| All unit-test related tasks build `spider_task_executor`, `unitTest` and `worker_test`, which are necessary to run unit | ||
| tests. | ||
|
|
||
| ## GitHub test workflow | ||
|
|
||
| A GitHub workflow `unit_tests.yaml` is set up to run unit test on push, pull request and every day. Currently, it only | ||
| runs unit tests without storage requirement. No newline at end of file |
There was a problem hiding this comment.
Can we replace from "Build and run unit tests" with the following?
## Running tests
You can use the following tasks to run the set of unit tests that's appropriate.
| Task | Description |
|-------------------------------|-------------------------------------------------------------------|
| `test:all` | Runs all unit tests. |
| `test:non-storage-unit-tests` | Runs all unit tests which don't require a storage backend to run. |
| `test:storage-unit-tests` | Runs all unit tests which require a storage backend to run. |
## GitHub unit test workflow
The [unit_tests.yaml][gh-workflow-unit-tests] GitHub workflow runs the unit tests on push,
pull requests, and daily. Currently, it only runs unit tests that don't require a storage backend.
If any tests show error messages for the connection function below, revisit
[setup section](#set-up-mysql-as-storage-backend) and verify that `cStorageUrl` was set correctly.And leave the storage->connect function after the snippet above.
And add a reference:
[gh-workflow-unit-tests]: ../.github/workflows/unit-tests.yamlThere was a problem hiding this comment.
Sorry, made some typos that I just edited now.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/testing.md (2)
36-41: Consider enhancing error handling documentationWhile the current instructions are helpful, consider adding:
- Common error messages that users might encounter
- Specific troubleshooting steps for each error scenario
Example addition:
+ Common errors: + - "Connection refused": Verify MySQL service is running + - "Access denied": Check user permissions and password + - "Unknown database": Ensure database name matches setup
43-48: Add information about manual workflow triggersThe workflow documentation accurately describes automated triggers, but consider adding information about manual workflow execution using the
workflow_dispatchevent.The [unit_tests.yaml][gh-workflow-unit-tests] GitHub workflow runs the unit tests on push, -pull requests, and daily. Currently, it only runs unit tests that don't require a storage backend. +pull requests, and daily. The workflow can also be triggered manually from the GitHub Actions tab. +Currently, it only runs unit tests that don't require a storage backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
build-tasks.yaml(1 hunks)docs/testing.md(1 hunks)test-tasks.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- build-tasks.yaml
- test-tasks.yaml
🔇 Additional comments (1)
docs/testing.md (1)
30-34: Well-structured task documentation!
The table clearly outlines the available test tasks and their purposes, making it easy for developers to choose the appropriate testing command.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
ci: Add tasks to run unit tests and GitHub workflow to run non-storage unit tests.
2 participants
Description
Add unit test to github workflow. Unit tests with "[storage]" tag requires MySQL set up to run, so they are excluded from the workflow.
Validation performed
Summary by CodeRabbit
New Features
Enhancements