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

CI: Run pytest through GitHub actions #2529

Merged
merged 7 commits into from
Sep 13, 2021
Merged

CI: Run pytest through GitHub actions #2529

merged 7 commits into from
Sep 13, 2021

Conversation

effigies
Copy link
Member

@effigies effigies commented Sep 8, 2021

Changes proposed in this pull request

I want to run pre-release tests on fMRIPrep to make sure that upcoming releases like numpy, nilearn or pybids aren't going to break things. GitHub actions seems the lightest-touch way of doing this.

Starting with stable tests. Will add pre-release if this works.

Started with stable tests, added pre-release tests. Added a test to ensure that we exercise pybids.

Documentation that should be reviewed

@effigies effigies force-pushed the ci/github_actions branch 2 times, most recently from ff019b1 to cd376ba Compare September 8, 2021 18:29
@pep8speaks
Copy link

pep8speaks commented Sep 8, 2021

Hello @effigies! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-09-08 20:45:00 UTC

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@10be2dc). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2529   +/-   ##
=========================================
  Coverage          ?   44.81%           
=========================================
  Files             ?       41           
  Lines             ?     3088           
  Branches          ?        0           
=========================================
  Hits              ?     1384           
  Misses            ?     1704           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10be2dc...db8bbf5. Read the comment docs.

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

This looks solid, thanks!

The only question is whether, going forward, we should try to use conda whenever possible instead of virtual environments and other solutions. As more and more of the tools we use are distributed with conda (and I'm thinking of ANTs as the most prominent example), we probably want CI and our Dockerfiles to leverage it more.

@effigies
Copy link
Member Author

If we're running tests that require these as dependencies, then sure. We don't currently have any pytest-runnable tests that depend on ANTs, do we?

@oesteban
Copy link
Member

If we're running tests that require these as dependencies, then sure. We don't currently have any pytest-runnable tests that depend on ANTs, do we?

No, that's why I think this PR can and should be merged. But for the future, I would lean towards standardizing the conda setup, even if we have to pay some increased installation price. Having a homogeneous setup across projects will help maintainability through time.

@effigies effigies merged commit 8b86156 into master Sep 13, 2021
@effigies effigies deleted the ci/github_actions branch September 13, 2021 16:14
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.

4 participants