Conversation
organise CI flow to be easier to follow
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
=======================================
Coverage 47.61% 47.61%
=======================================
Files 3 3
Lines 42 42
Branches 4 4
=======================================
Hits 20 20
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds CI coverage for testing this repository against both the latest MUSE2 release and a build from the MUSE2 main branch, factoring the setup/verification logic into reusable workflows and composite actions.
Changes:
- Refactors CI to call a reusable workflow that runs the test matrix against a selected MUSE2 source (
releaseormain). - Adds composite actions to (a) install MUSE2 from the latest release, (b) build/install MUSE2 from
main, and (c) verifyMUSE2_PATHis set. - Updates the main CI workflow to run both “release” and “main” test suites.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/test-with-muse2.yml |
New reusable workflow that installs deps, sets up MUSE2 (main/release), verifies, runs pytest, and uploads coverage for release/Linux. |
.github/workflows/ci.yml |
Refactors CI to run two jobs (release + main) via the reusable workflow. |
.github/actions/verify-muse2/action.yml |
New composite action to validate MUSE2_PATH. |
.github/actions/setup-muse2-release/action.yml |
New composite action to download/extract latest MUSE2 release asset and set MUSE2_PATH. |
.github/actions/setup-muse2-main/action.yml |
New composite action to cargo install MUSE2 from the main branch and set MUSE2_PATH. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Setup MUSE2 from main | ||
| if: inputs.muse2_source == 'main' | ||
| uses: ./.github/actions/setup-muse2-main | ||
| with: | ||
| muse2_exe: ${{ matrix.muse2_exe }} |
There was a problem hiding this comment.
Because MUSE2 setup is entirely conditional on inputs.muse2_source, any unexpected value will skip both setup steps and then fail later in verify-muse2 with a misleading “executable not found” error. Consider adding an explicit validation step early in the job (and a clear error message) to enforce allowed values (e.g. main/release).
|
|
||
| - name: Upload coverage to Codecov | ||
| # Latest linux release only - they should all be the same | ||
| if: runner.os == 'Linux' && inputs.muse2_source == 'release' |
There was a problem hiding this comment.
CODECOV_TOKEN is declared as an optional secret for this reusable workflow, but the Codecov step is configured with fail_ci_if_error: true and always passes the token input. If the token isn’t available (e.g. fork PRs), this can cause CI failures unrelated to the tests—consider gating the step on the token being present, omitting the token input when empty, or not failing CI on upload errors in those contexts.
| if: runner.os == 'Linux' && inputs.muse2_source == 'release' | |
| if: runner.os == 'Linux' && inputs.muse2_source == 'release' && secrets.CODECOV_TOKEN != '' |
| @@ -0,0 +1,24 @@ | |||
| name: Verify MUSE2 installation | |||
| description: Validate that MUSE2_PATH points to an executable file for the current OS | |||
There was a problem hiding this comment.
The action description says it validates that MUSE2_PATH points to an executable, but the checks only assert the path is an existing file (Linux/macOS: -f; Windows: Test-Path -PathType Leaf). Either update the checks to verify executability (e.g., -x on Linux/macOS) or adjust the description to match what is actually validated.
| description: Validate that MUSE2_PATH points to an executable file for the current OS | |
| description: Validate that MUSE2_PATH points to an existing file for the current OS |
| if [[ ! -f "$MUSE2_PATH" ]]; then | ||
| echo "::error::MUSE2 executable not found at: $MUSE2_PATH" |
There was a problem hiding this comment.
On Linux/macOS the verification uses [[ ! -f "$MUSE2_PATH" ]], which won’t catch the case where the file exists but is not executable. Consider checking -x (or both -f and -x) so the failure is caught here with a clearer error instead of later when trying to run MUSE2.
| if [[ ! -f "$MUSE2_PATH" ]]; then | |
| echo "::error::MUSE2 executable not found at: $MUSE2_PATH" | |
| if [[ ! -f "$MUSE2_PATH" || ! -x "$MUSE2_PATH" ]]; then | |
| echo "::error::MUSE2 executable not found or is not executable at: $MUSE2_PATH" |
| run: | | ||
| cargo install \ | ||
| --git https://github.com/EnergySystemsModellingLab/MUSE2 \ | ||
| --rev main \ |
There was a problem hiding this comment.
cargo install is being pointed at the main branch via --rev main, but --rev is intended for a specific revision (typically a commit SHA). Use --branch main (or pin to a commit SHA) to avoid ambiguity/breakage, and consider adding --locked so dependency resolution is reproducible and less likely to cause CI flakes over time.
| --rev main \ | |
| --branch main \ | |
| --locked \ |
| test_main: | ||
| name: Test against MUSE2 main | ||
| uses: ./.github/workflows/test-with-muse2.yml | ||
| with: | ||
| muse2_source: main | ||
| secrets: inherit |
There was a problem hiding this comment.
This adds a full “build MUSE2 from main” test job to the standard CI on every push/PR, but the PR description/issue calls out running the main-branch build periodically. If the intent is periodic coverage, consider gating test_main behind a schedule/manual trigger (or making it conditional) to avoid significantly increasing CI duration and external network/build load on every PR.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Upload coverage to Codecov | ||
| # Latest linux release only - they should all be the same | ||
| if: runner.os == 'Linux' && inputs.muse2_source == 'release' | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| files: ./coverage.xml | ||
| fail_ci_if_error: true |
There was a problem hiding this comment.
CODECOV_TOKEN is declared as an optional secret for this reusable workflow, but the Codecov step will still run (on Linux release) and passes token: ${{ secrets.CODECOV_TOKEN }} with fail_ci_if_error: true. In contexts where secrets aren’t available (e.g., PRs from forks) or the caller doesn’t provide the token, this can cause the workflow to fail unexpectedly. Consider updating the if: guard to also require a non-empty token (or skip uploads on forked PRs), or make the token required if failures are desired.
We were only testing the analysis tools against the latest releases of MUSE2 which won't be that frequent - therefore we wanted to also have testing against a build of the current main branch we can run periodically. I have added this to the CI and refactored a bit so it's hopefully easier to follow
Description
Close #25
Type of change
Key checklist
python -m pytest)pre-commit run --all-files)Further checks
(Indicate issue here: # (issue))