-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add GitHub Actions. #698
Add GitHub Actions. #698
Conversation
Codecov Report
@@ Coverage Diff @@
## master #698 +/- ##
==========================================
+ Coverage 68.42% 68.57% +0.14%
==========================================
Files 41 41
Lines 4162 4162
Branches 1025 1025
==========================================
+ Hits 2848 2854 +6
+ Misses 1103 1097 -6
Partials 211 211
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have some suggestions to improve the testing.
on: | ||
push: | ||
branches: | ||
- 'release/*.*.*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
publish-packages.yml
is not tested in this PR because it does not match this branch name. You could remove this condition temporarily to test it, or remove it permanently. GitHub Actions provides many runners, so the additional testing time is not an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script matches the testing workflows on signac-dashboard, which were copied from signac. I think I trust this workflow from those two repos, so I was going to leave it “untested” until 0.23.0 release time. I don’t think we want to publish test packages on every branch/commit.
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [ubuntu-latest, macos-latest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no testing on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows is not fully supported by signac-flow. We have a number of features like command line operations and “with job” that inject UNIX-specific things like trap commands. About 80% of our test suite passes on Windows, if I recall correctly, but tests of job submission and a few other categories fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an area for improvement — we should mark some tests as skip on Windows, and/or block those features with explicit errors on Windows. I don’t think we have received many complaints about support, but I know we have some users of the package’s partial functionality on Windows so I don’t want to explicitly disallow installation of the package on Windows as an unsupported platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we can improve here, but I think we are okay to merge this without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few notes!
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [ubuntu-latest, macos-latest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we can improve here, but I think we are okay to merge this without it.
- name: Test with pytest | ||
run: | | ||
pytest --cov=flow --cov-config=setup.cfg --cov-report=xml tests/ -v | ||
- uses: codecov/codecov-action@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have CI fail if codecov report upload fails? Sometimes it can be flaky, not sure if we have ran into that with this repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to worry about this. It's probably fine.
Description
Migrates signac-flow's CI to use GitHub Actions.
This PR drops coverage for a few things that I think are non-essential:
pip install signac signac-flow
andconda install signac signac-flow
signac
(which is currently disabled because it's broken until we release 2.0)release/.*
branches.If another contributor wishes to add these to GitHub Actions, that would be fine.
Before merging, a repo administrator will need to update the CI checks used for branch protections.
Checklist: