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

Remove redundant integration tests #259

Merged
merged 3 commits into from
Jun 19, 2020
Merged

Remove redundant integration tests #259

merged 3 commits into from
Jun 19, 2020

Conversation

eurunuela
Copy link
Collaborator

Closes #258

Proposed Changes

  • Remove redundant integration tests to gain in testing speed.

@eurunuela eurunuela added the Testing This is for testing features, writing tests or producing testing code label Jun 19, 2020
@eurunuela
Copy link
Collaborator Author

eurunuela commented Jun 19, 2020

I don't know what's going on with Travis... some numpy related issues appeared and I honestly don't know why. Nothing has changed in the installation process... any idea @rmarkello ?

Anyway, the tests are much faster when removing the redundant ones (see here and they took 3:47 on my laptop). I checked codecov and we would have a -0.52% decrease in coverage (to 92.97%). The interfaces folder would still be covered at 98.83%, which means that removing the ACQ and TXT integration tests has no impact on txt.py and acq.py.

This means that the only ones below 90% would be bids.py (which will probably be above 90% when #254 is merged), due.py and phys2bids.py. I think it should be easy to increase coverage on them with silly unit tests without compromising testing speed.

So, I think merging this PR would improve our testing suite.

@vinferrer
Copy link
Collaborator

LGTM, i think this is ready for merge

@eurunuela
Copy link
Collaborator Author

eurunuela commented Jun 19, 2020

We will have a bit of a decrease in overall coverage (down to 91%) but I expect #254 to increase that (bids.py from 73% to 98%).

@eurunuela
Copy link
Collaborator Author

Just to keep this written somewhere:

As per discussion over Zoom with @smoia and @vinferrer , I added the acq test back and merged the others into one single test (heuristic with multi-freq txt file and checks on the logger). The heuristic one takes 8 seconds on my laptop while the acq one takes around 40 seconds. I'll check if we have a short acq file on OSF and update if I find one.

Anyway, this is a very big decrease in testing time.

@eurunuela eurunuela marked this pull request as ready for review June 19, 2020 11:57
Copy link
Collaborator

@vinferrer vinferrer left a comment

Choose a reason for hiding this comment

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

Let's merge

@eurunuela eurunuela assigned vinferrer and unassigned smoia Jun 19, 2020
@eurunuela eurunuela merged commit 671f9c9 into physiopy:master Jun 19, 2020
@eurunuela eurunuela deleted the enh/remove_tests branch August 25, 2020 05:50
@smoia smoia added the released This issue/pull request has been released. label Oct 14, 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.

Reduce the amount of integration tests to make tests faster
3 participants