Skip to content
This repository has been archived by the owner on Dec 20, 2024. It is now read-only.

MAINT: Revision of GHA migration #230

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Sep 30, 2024

Revises the GHA migration to make sure that tests run:

  • Added dataset download using datalad (as it was the case in Circle)
  • Improved the tox config to ensure environment variables are considered
  • Added execution of pytest on the test/ folder.
  • Does not test on a matrix of pythons because the python version should not introduce differences (to consider when we have code where it could be a problem).

IMHO, the addition of tox deviates from other projects (and as far as I understand, we were not planning on using tox) and has introduced some complexity. In particular, I had to set one test to skip because of (I suspect) some minimal environment change tox introduces.

For now, I wanted to focus on the download of data and execution of all tests. Future PRs can address the removal of tox if we finally decide against it.

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.35%. Comparing base (a7880a3) to head (7fb31c8).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #230       +/-   ##
===========================================
+ Coverage   32.97%   53.35%   +20.37%     
===========================================
  Files          17       17               
  Lines         849      849               
  Branches      149      143        -6     
===========================================
+ Hits          280      453      +173     
+ Misses        558      369      -189     
- Partials       11       27       +16     

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

@oesteban oesteban force-pushed the maint/fix-gha-transition branch 11 times, most recently from 7fb2086 to 75bb399 Compare September 30, 2024 15:58
@oesteban oesteban force-pushed the maint/fix-gha-transition branch 3 times, most recently from 5914e2b to 11b5b8b Compare September 30, 2024 18:42
@oesteban oesteban force-pushed the maint/fix-gha-transition branch from 11b5b8b to 7fb31c8 Compare September 30, 2024 19:06
@oesteban oesteban marked this pull request as ready for review September 30, 2024 19:14
Copy link
Collaborator

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Thanks for doing this.

I apologize for not having inspected closely the CircleCI config file, and for inadvertently discarding the data download and testing the test/ folder scripts. Now I realize that the CI duration should have been indicative of this.

Sorry if the adoption of tox made things more complicated; assumed there was some consensus as it had been adopted on other NiPreps repositories. Can revisit later.

Merging for not to go forward.

@jhlegarreta jhlegarreta merged commit 4a6cf72 into main Sep 30, 2024
11 checks passed
@jhlegarreta jhlegarreta deleted the maint/fix-gha-transition branch September 30, 2024 23:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants