Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverage only runs runs when meaningful files are changed #468

Merged
merged 16 commits into from
Jun 14, 2024

Conversation

okBrian
Copy link
Contributor

@okBrian okBrian commented Jun 10, 2024

Description

Added https://github.com/dorny/paths-filter to CI based on @sbryngelson Pull Request #461
The Filter is modulated on purpose for future use as you can specify which grouping you want as seen in test.yml & coverage.yml

Type of change

  • Something else

Scope

  • This PR comprises a set of related changes with a common goal

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?

  • Edited the CMakeFile, then tested to make sure the filter caught the changes, and the tests would run.
  • Edited no files and sent changes and commented away some parts of the filter to ensure the test would be skipped.

Test Configuration:

Github Actions on my fork

Checklist

  • I have added comments for the new code
  • I ran ./mfc.sh format before committing my code
  • This PR does not introduce any repeated code (it follows the DRY principle)
  • I cannot think of a way to condense this code and reduce any introduced additional line count

@sbryngelson
Copy link
Member

can be reviewed and merged by @henryleberre

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.91%. Comparing base (596ef8b) to head (08262e1).

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henryleberre
Copy link
Member

Is there a reason you can't use the paths: feature like we do for on: [push] ?

@okBrian
Copy link
Contributor Author

okBrian commented Jun 12, 2024

The main reason I suggested using https://github.com/dorny/paths-filter was because the path applied to pull request, push, and workflow_dispatch.
I don't think their is a way to do something like this

on: 
  [push, pull_request, workflow_dispatch]
      paths:
      - '**.f90'
      - '**.fpp'
      - '**.py'
      - '**.yml'
      - 'mfc.sh'
      - 'golden.txt'
      - 'CMakeLists.txt'
      - 'requirements.txt'

If we can't do something similar to above we are repeating a lot of code, which is something Spencer was trying to change in his pr I think.
Also since the path filter can read from file all CI can share the same file (like coverage and test) and just filter what it needs.

@sbryngelson
Copy link
Member

indeed the above syntax doesn't work, though I do notice that you have to repeat your filtering statement in all of the workflows in tests.yml ...

@okBrian
Copy link
Contributor Author

okBrian commented Jun 12, 2024

This is true, but in the future if we have different jobs in the same files that want to check different files we can specify that it in the conditional like in this example: https://github.com/GoogleChrome/web.dev/blob/3a57b721e7df6fc52172f676ca68d16153bda6a3/.github/workflows/lint-workflow.yml#L26 . This feature might be useful if we add more complex tests.

@henryleberre
Copy link
Member

I was assuming that Github supported YAML anchors.. but they don't. actions/runner#1182 has been open since 2021 and has 1k upvotes. The last comment in that thread gives an example of how I was imagining using it.

@sbryngelson sbryngelson merged commit 2810b09 into MFlowCode:master Jun 14, 2024
19 of 21 checks passed
@okBrian okBrian deleted the testFilter branch June 17, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants