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

Add test to check that phys2bids output is BIDS compatible #362

Merged
merged 34 commits into from
Dec 4, 2020

Conversation

vinferrer
Copy link
Collaborator

@vinferrer vinferrer commented Dec 3, 2020

Closes #355

This addition to the config file sets a test that runs bids_validator on the tutorial file

Proposed Changes

  • add tests for bids_validator

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@vinferrer
Copy link
Collaborator Author

Okay, I think this is working, l am gonna leave it as a draft until the PR mentioned in this issue: https://neurostars.org/t/quick-validation-failed-on-physiological-files/17611/6 gets approved and a new release is passed. However as you can see it is doing exactly what we want. Now that the bids-validator doesn't accept physiological only bids dataset it gives an error and breaks the test.

@vinferrer
Copy link
Collaborator Author

vinferrer commented Dec 3, 2020

@eurunuela How do you use the makeenv_37 enviroment work in your others jobs? I tried to recreate the steps but since i couldn't i reisntall phys2bids in my bids_validate job

@eurunuela
Copy link
Collaborator

@eurunuela How do you use the makeenv_37 enviroment work in your others jobs? I tried to recreate the steps but since i couldn't i reisntall phys2bids in my bids_validate job

I think you're missing this:

- bids_validate
    requires:
      - makeenv_37

Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @vinferrer . I set it as "request changes" until the test passes. Then I'll approve.

Edit: I forgot to mention that if you think this is ready for review, you should click on the button 😉

@vinferrer
Copy link
Collaborator Author

vinferrer commented Dec 3, 2020

LGTM! Thank you @vinferrer . I set it as "request changes" until the test passes. Then I'll approve.

Edit: I forgot to mention that if you think this is ready for review, you should click on the button 😉

No, it's not ready, we have to wait for the bids-validator update that accepts only physiological datasets, then i will push an empty commit to see if the test passes or if there is something else to correct

@vinferrer vinferrer marked this pull request as ready for review December 4, 2020 09:10
@vinferrer
Copy link
Collaborator Author

I think bids-validator people are not gonna allow only physio datasets, So I decided to put a dummy file

@vinferrer vinferrer requested review from smoia and eurunuela December 4, 2020 09:11
@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #362 (8febea1) into master (fa5238c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #362   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files           8        8           
  Lines         840      840           
=======================================
  Hits          795      795           
  Misses         45       45           
Impacted Files Coverage Δ
phys2bids/bids.py 98.05% <100.00%> (ø)

Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

LGTM!

@eurunuela eurunuela added the Testing This is for testing features, writing tests or producing testing code label Dec 4, 2020
@eurunuela eurunuela changed the title add circle ci test for BIDS_VALIDATOR Add BIDS_VALIDATOR test Dec 4, 2020
Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

This is super useful. Thank you @vinferrer !

@vinferrer vinferrer merged commit 0e0ec5d into physiopy:master Dec 4, 2020
@smoia smoia changed the title Add BIDS_VALIDATOR test Add test to check that phys2bids output is BIDS compatible Dec 4, 2020
@smoia
Copy link
Member

smoia commented Dec 9, 2020

🚀 PR was released in 2.3.2 🚀

@smoia smoia added the released This issue/pull request has been released. label Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released. Testing This is for testing features, writing tests or producing testing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test to pass a BIDS validator
3 participants