MFlowCode / MFC

Exascale simulation of multiphase/physics fluid dynamics
https://mflowcode.github.io
MIT License
132 stars 56 forks source link

coverage only runs when meanful files are changed (duplicates code fr… #461

Closed sbryngelson closed 3 weeks ago

sbryngelson commented 3 weeks ago

…om test.yml). This removes the other on: commands because I couldn't figure out the right syntax.

Description

Changed codecov to only run when meaningful files are changed.

This code probably needs improvement from @henryleberre and/or @okBrian. It duplicates paths from test.yml and gits rid of the push and workflow_dispatch events because I couldn't figure out the right syntax for them.

Type of change

Scope

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 57.91%. Comparing base (4dd7f25) to head (14b8299).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #461 +/- ## ======================================= Coverage 57.91% 57.91% ======================================= Files 55 55 Lines 14230 14230 Branches 1854 1854 ======================================= Hits 8242 8242 Misses 5452 5452 Partials 536 536 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

okBrian commented 3 weeks ago

Instead of using GitHub built-in path, we can use something like this https://github.com/dorny/paths-filter . This should reduce possible repeated code since we don't need to use path on each pull request, workflow dispatch, and push, but instead have a single filter step. Here's a example I created that worked on my branch:

name: Coverage Check

on: [push, pull_request, workflow_dispatch]

jobs:
  run:
    name: Coverage Test on CodeCov
    runs-on: "ubuntu-latest"
    steps:
      - name: Setup Ubuntu
        run: |
            sudo apt update -y
            sudo apt install -y tar wget make cmake gcc g++ python3 python3-dev "openmpi-*" libopenmpi-dev

      - name: Checkouts
        uses: actions/checkout@v4

      - name: Filters
        uses: dorny/paths-filter@v3
        id: filter
        with: 
          # tests/ should encompass the golden and golden-metadata.txt
          filters: |
            root:
              - 'mfc.sh'
              - 'CMakeLists.txt'
            fortran:
              - '**/*.f90'
              - '**/*.fpp'
            python:
              - '**/*.py'
              - 'toolchain/requirements.txt'
            yml:
              - '**/*.yml'
            tests:
              - 'tests/**'

      - name: Build
        run: /bin/bash mfc.sh build -j $(nproc) --gcov

      - name: Test
        run: /bin/bash mfc.sh test -a -j $(nproc)

      - name: Upload coverage reports to Codecov
        uses: codecov/codecov-action@v4.0.1
        with:
          token: ${{ secrets.CODECOV_TOKEN }}

You can even create custom filters like for documentation.

sbryngelson commented 3 weeks ago

So, will this kill the job if none of the files in the filter are modified? If so, great!

I do see that this is only used in coverage.yml, but we would want to use the same filter in tests.yml. Can we do that (so as to not duplicate code)?

okBrian commented 3 weeks ago

Yea, we can create a filter file so we can reuse a filter in multiple tests as seen here https://github.com/getsentry/sentry/blob/2ebe01feab863d89aa7564e6d243b6d80c230ddc/.github/workflows/backend.yml#L40 https://github.com/getsentry/sentry/blob/2ebe01feab863d89aa7564e6d243b6d80c230ddc/.github/file-filters.yml

sbryngelson commented 3 weeks ago

perfect can you add to this PR for this? (or create your own and I'll close this one)

okBrian commented 3 weeks ago

Okay I'll add it to this one

okBrian commented 3 weeks ago

I don't think I have the permissions to add to this pull request since I can't even fetch the changes from your fork, assuming I am doing this correctly, so I will just make another pull request when I finish.

sbryngelson commented 3 weeks ago

closing in favor of #468