Skip to content

add muse2 main branch install to ci#50

Open
Aurashk wants to merge 3 commits intomainfrom
add-muse2-main-to-ci
Open

add muse2 main branch install to ci#50
Aurashk wants to merge 3 commits intomainfrom
add-muse2-main-to-ci

Conversation

@Aurashk
Copy link
Copy Markdown
Contributor

@Aurashk Aurashk commented Apr 24, 2026

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

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass (eg. python -m pytest)
  • Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added or an issue has been opened to tackle that in the future.
    (Indicate issue here: # (issue))

organise CI flow to be easier to follow
Copilot AI review requested due to automatic review settings April 24, 2026 12:48
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.61%. Comparing base (790a01a) to head (9cfb02c).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (release or main).
  • Adds composite actions to (a) install MUSE2 from the latest release, (b) build/install MUSE2 from main, and (c) verify MUSE2_PATH is 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.

Comment on lines +45 to +49
- name: Setup MUSE2 from main
if: inputs.muse2_source == 'main'
uses: ./.github/actions/setup-muse2-main
with:
muse2_exe: ${{ matrix.muse2_exe }}
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.

- name: Upload coverage to Codecov
# Latest linux release only - they should all be the same
if: runner.os == 'Linux' && inputs.muse2_source == 'release'
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if: runner.os == 'Linux' && inputs.muse2_source == 'release'
if: runner.os == 'Linux' && inputs.muse2_source == 'release' && secrets.CODECOV_TOKEN != ''

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,24 @@
name: Verify MUSE2 installation
description: Validate that MUSE2_PATH points to an executable file for the current OS
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +11
if [[ ! -f "$MUSE2_PATH" ]]; then
echo "::error::MUSE2 executable not found at: $MUSE2_PATH"
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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"

Copilot uses AI. Check for mistakes.
run: |
cargo install \
--git https://github.com/EnergySystemsModellingLab/MUSE2 \
--rev main \
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
--rev main \
--branch main \
--locked \

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/ci.yml
Comment on lines +17 to +22
test_main:
name: Test against MUSE2 main
uses: ./.github/workflows/test-with-muse2.yml
with:
muse2_source: main
secrets: inherit
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings April 24, 2026 14:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +64 to +71
- 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
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add main muse2 branch to CI

3 participants