chore: enhance testing CI/CD #117
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request updates the testing configuration and workflow across the project. In the CI/CD workflow, the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as CI/CD Pipeline
participant Executor as Test Executor
participant Summary as JUnit Summary Step
participant Uploader as Coverage Uploader
Dev->>CI: Push commit with changes
CI->>Executor: Start Unit-Tests job
Executor->>Executor: Run tests with "pixi run -e ${{ matrix.env }} test -s -vv"
Executor->>Summary: Always execute test-summary step
Summary-->>CI: Produce JUnit test report
CI->>Uploader: Conditionally upload coverage report
Uploader-->>CI: Coverage artifact processed
Poem
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. 🪧 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 (
|
…w for closed pull requests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/pytest.ini (1)
63-69: Consider adding more specific warning filters.While the current warning filters are good, consider being more specific about which DeprecationWarnings to ignore.
filterwarnings = - ignore::DeprecationWarning + ignore::DeprecationWarning:pytest.* + ignore::DeprecationWarning:_pytest.* # Add specific warning messages to ignore here ignore::UserWarning:pydicom.*.github/workflows/ci-cd.yml (1)
44-50: Fix trailing spaces in YAML.Remove trailing spaces to maintain consistent formatting.
- name: JUnit Test Summary id: pytest-summary uses: test-summary/action@v2 - with: + with: paths: .cache/test-results/**/*.xml show: "fail,skip" if: always()🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci-cd.yml(2 hunks)config/pytest.ini(1 hunks)pyproject.toml(1 hunks)tests/test_feature_extraction.py(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/ci-cd.yml
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit-Tests (windows-latest, py312)
- GitHub Check: Unit-Tests (windows-latest, py311)
- GitHub Check: Unit-Tests (windows-latest, py310)
- GitHub Check: Unit-Tests (macos-14, py312)
- GitHub Check: Unit-Tests (macos-14, py311)
- GitHub Check: Unit-Tests (macos-14, py310)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (macos-latest, py310)
🔇 Additional comments (7)
tests/test_feature_extraction.py (2)
47-57: LGTM! Improved fixture management with module scope and safer cleanup.The changes enhance test reliability by:
- Using module scope to reduce fixture setup/teardown overhead
- Adding existence check before cleanup to prevent errors during parallel test execution
58-68: LGTM! Consistent fixture improvements applied.The same improvements have been correctly applied to the lung4DMetadataPath fixture.
config/pytest.ini (3)
1-11: LGTM! Well-structured basic configuration.The configuration properly sets:
- Minimum pytest version for compatibility
- Cache directory for test artifacts
- Progress-style console output for better visibility
12-45: LGTM! Comprehensive test execution configuration.The addopts section effectively configures:
- Detailed test output with --showlocals
- Coverage reporting in multiple formats
- Parallel test execution with load grouping
- JUnit XML reporting for CI integration
51-61: LGTM! Clear test discovery patterns.Well-defined patterns for:
- Test paths
- Excluded directories
- Test file naming conventions
pyproject.toml (1)
76-78: LGTM! Simplified test execution configuration.Test command now properly uses the centralized pytest.ini configuration, making it easier to maintain and modify test settings.
.github/workflows/ci-cd.yml (1)
32-34: LGTM! Enhanced test execution visibility.Added verbosity flags (-s -vv) to improve test output in CI logs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
===========================================
+ Coverage 50.25% 76.45% +26.19%
===========================================
Files 36 41 +5
Lines 1546 2009 +463
===========================================
+ Hits 777 1536 +759
+ Misses 769 473 -296 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/cleanup_caches.yml (4)
1-6: Filter to merged pull requests only
Triggering on every closed pull request may clear caches for PRs that were simply closed without merging. To avoid unnecessary deletions, add a merge check at the job level.on: pull_request: types: - closed jobs: cleanup: + if: ${{ github.event.pull_request.merged == true }} runs-on: ubuntu-latest
12-15: Use the configured repository context when listing caches
You defineGH_REPO, but thegh cache listinvocation doesn’t reference it. Either remove the unused env var or explicitly pass--repo $GH_REPOso you’re cleaning the correct repository.- cacheKeysForPR=$(gh cache list --ref $BRANCH --limit 100 --json id --jq '.[].id') + cacheKeysForPR=$(gh cache list --repo $GH_REPO --ref $BRANCH --limit 100 --json id --jq '.[].id')
16-23: Leverage step-levelcontinue-on-errorinstead ofset +e
Silencing errors mid-script can mask unexpected failures. It’s more idiomatic to usecontinue-on-error: trueon the step itself.- - name: Cleanup - run: | - ## Setting this to not fail the workflow while deleting cache keys. - set +e + - name: Cleanup + continue-on-error: true + run: | echo "Fetching list of cache key" cacheKeysForPR=$(gh cache list --repo $GH_REPO --ref $BRANCH --limit 100 --json id --jq '.[].id') echo "Deleting caches..."
24-27: Add explicit write permissions for cache operations
To delete caches programmatically, theGITHUB_TOKENneedsactions: writepermission. Declare this at the top of the workflow for clarity and security.+permissions: + actions: write name: cleanup caches by a branch on: pull_request:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/ci-cd.yml.disabled(0 hunks).github/workflows/cleanup_caches.yml(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci-cd.yml.disabled
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (1)
.github/workflows/cleanup_caches.yml (1)
9-10:✅ Verification successful
Verify GH CLI version supports cache commands
gh cache listandgh cache deleteare relatively new subcommands. Ensure the Ubuntu runner’s GitHub CLI version includes them to avoid “command not found” errors.
🏁 Script executed:
#!/bin/bash # Confirm installed GH CLI version and availability of cache commands gh --version gh cache --helpLength of output: 686
GitHub CLI cache commands are supported on ubuntu-latest
Verified thatgh version 2.62.0on the Ubuntu runner includes bothgh cache listandgh cache delete. No changes required.
2 participants
copied over the pytest config from med-imagetools
can easily view errors in gha during pytest without going into the action logs
small fix to the fixtures that delete temporary paths and are used by multiple tests (can cause errors during parallel runs)
also cleanups for each PRs gha caches
Summary by CodeRabbit
Summary by CodeRabbit