Makes txt testing faster

[vinferrer]

Closes #172
Shortens the txt.py testing

Proposed Changes

• Now the tested acq file has only the trigger and other channel and the length of the channels is 1048560
• All the downloads are done at the beggining and then the functions get the needed parameters. The only exception is test_populate_phys_input because the tested function has to read and this was giving problems if the file wasn't download inside the function
• Testing reduced from 7 to 5 min

[codecov]

Codecov Report

Merging #178 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #178   +/-   ##
=======================================
  Coverage   94.15%   94.15%           
=======================================
  Files           7        7           
  Lines         565      565           
=======================================
  Hits          532      532           
  Misses         33       33           

[eurunuela]

LGTM! 2 minutes faster is quite a thing! Do you know which of the tests is taking the longest to run?

I think that #162 could help with reducing testing time even more. What do you think @vinferrer ?

[rmarkello]

Hey @vinferrer! Thanks so much for getting this started—I'm really glad to hear that the testing time is down ~25%!

My primary concern about this new implementation is that it might lead to difficulties detecting bugs in the codebase. That is, if there's an issue in any of the code that you've moved outside functions (e.g., if a file is accidentally moved or deleted from OSF, or there's an issue in the txt.read_header_and_channels() function for one of the test files), pytest will error during test collection rather than during the tests themselves. This makes debugging significantly harder—we don't get the helpful information pytest tries to give us about what tests failed / passed. (It will tell us what file / line caused the error during collection, but the whole testing suite aborts before any tests are run. You can see what this is like by modifying one of the OSF URLs in the download section of test_txt.py right now (e.g., change 'https://osf.io/4yudk/download' to 'https://osf.io/INVALID/download') and then running pytest on everything.)

One potential solution (as I described in #172) would be to use pytest.fixtures and add discrete download functions. When you do that, pytest is pretty verbose about exactly where the error is, and the entire test suite will run (even if some of the tests /fixtures fail). I'm happy to discuss alternatives or implementation specifics in more detail, but I think the current set-up (with much of the code living "freeform" inside the testing modules) isn't ideal.

[rmarkello]

Since I haven't done much codestuff for phys2bids in a while would you be willing for me to maybe make a PR on your PR, implementing some of my suggestions? Let me know if you'd be open to that 😄

[vinferrer]

I see, let me code a bit tomorrow, I want a try do it by my self

[vinferrer]

LGTM! 2 minutes faster is quite a thing! Do you know which of the tests is taking the longest to run?

I think that #162 could help with reducing testing time even more. What do you think @vinferrer ?

That we can treat in another PR, but sure reducing check_multifreq time would be great

[vinferrer]

Hey @vinferrer! Thanks so much for getting this started—I'm really glad to hear that the testing time is down ~25%!

My primary concern about this new implementation is that it might lead to difficulties detecting bugs in the codebase. That is, if there's an issue in any of the code that you've moved outside functions (e.g., if a file is accidentally moved or deleted from OSF, or there's an issue in the txt.read_header_and_channels() function for one of the test files), pytest will error during test collection rather than during the tests themselves. This makes debugging significantly harder—we don't get the helpful information pytest tries to give us about what tests failed / passed. (It will tell us what file / line caused the error during collection, but the whole testing suite aborts before any tests are run. You can see what this is like by modifying one of the OSF URLs in the download section of test_txt.py right now (e.g., change 'https://osf.io/4yudk/download' to 'https://osf.io/INVALID/download') and then running pytest on everything.)

One potential solution (as I described in #172) would be to use pytest.fixtures and add discrete download functions. When you do that, pytest is pretty verbose about exactly where the error is, and the entire test suite will run (even if some of the tests /fixtures fail). I'm happy to discuss alternatives or implementation specifics in more detail, but I think the current set-up (with much of the code living "freeform" inside the testing modules) isn't ideal.

Talking about this, should we use the pytest.fixtures functions for dowloading or can we add also the common preproccessing before the asserts?

[vinferrer]

Okay guys I refactored the code and now I am using @pytest.fixture the testing time is more or less the same (5 min) except if codecov gets executed, then is 7. I think you can do the review now @rmarkello.

[rmarkello]

Thanks so much for making these edits, @vinferrer ! 💪

I have a few more thoughts / suggestions for the test suite overall, but I don't want to hold this PR up so I think we should merge this in !

[smoia]

@rmarkello, there is no rush in merging this in, if you want to make some suggestions!
However, if it's about testing in general, I would ask you to open another issue/PR/both (and merge this in, please!).