Consolidate testing suite

[rmarkello]

Summary

With @vinferrer's updates to the test suite (via #178) merged (🙌), I thought it might be worth re-visiting the test suite in a more general sense. It's evolved pretty rapidly as development has happened on phys2bids and I think it might be worth doing a bit of consolidation. I wanted to make this issue to discuss ways that we might do that, and offer a few initial suggestions. (I will also open a PR shortly after posting this issue implementing some of these suggestions to give a better idea of what this would look like in practice.)

Additional detail

1. Testing files

First, I wanted to determine what the differentiation is between the files that are included in the phys2bids/tests/data directory versus the files that are on OSF and are downloaded in the test suite. Should we just move all the files in that directory onto OSF, or is there some reason they're included? For example, I can't find the Test_belt_pulse_multifreq.acq or Test_multiscan100Hz_trig_samefreq_header.txt that are in that directory used anywhere in phys2bids, suggesting they should possibly be removed.

I understand wanting to have some files for the documentation (e.g., tutorial_file.{txt,json}), but I would be in favor of moving the remainder to OSF. If we're going to use a lot of files in documentation then we could consider making some user-facing "fetcher" functions (e.g., fetch_tutorial_data()). If it's all just for testing then we don't have to worry as much, though!

2. Using pytest.fixture

@vinferrer took the first steps in using pytest.fixture functionality to consolidate downloading the files in test_txt.py, but I think we can use this to our advantage on a wider scale in the testing suite. pytest allows a conftest.py file in which you can put test-wide fixtures. I think we could move all the test downloads to that file and then they would be accessible to all the test_X.py modules. E.g.,

@pytest.fixture(scope='session')
def multifreq_acq_file():
    url = 'https://osf.io/9a7yv/download'
    test_path = resource_filename('phys2bids', 'tests/data')
    test_filename = 'Test_belt_pulse_multifreq.acq'
    test_full_path = os.path.join(test_path, test_filename)
    wget.download(url, test_full_path)
    return test_full_path

Since the download steps are identical for each file I would recommend we make a small helper function, too:

def fetch_file(url, filename):
    url = 'https://osf.io/{}/download'.format(url)
    test_path = resource_filename('phys2bids', 'tests/data')
    full_path = os.path.join(test_path, filename)
    if not os.path.isfile(full_path):
        wget.download(url, full_path)
    return full_path


@pytest.fixture(scope='session')
def multifreq_acq_file():
    return fetch_file('9a7yv', 'Test_belt_pulse_multifreq.acq')

Then we can have module-specific pytest.fixture functions that e.g., load these files (as @vinferrer did with test_read_header_and_channels() in test_txt.py.

(I would also suggest we download files to a temporary directory rather than to the phys2bids/tests/data directory, since that ends up cluttering the local git repo. We could use pytest's tmp_path functionality to do this.)

3. Using pytest.mark.parametrize

There are a lot of times where we're using the same file and just testing slightly different parameters (e.g., changing the units in the header of a file and checking that the output of some function changes in response). This is the perfect use case for pytest.mark.parametrize! Having looked at test_txt.py most recently I know there are some functions where this could be used, but taking a quick peek at the test suite more broadly I think there are other places that would benefit from this, as well. E.g., looking at test_txt.test_process_acq(), I think something like this could work:

@pytest.mark.parametrize('units, expected', [
    ('1 msec/sample', 1000),
    ('0.01 sec/sample', 100),
    ('1 µsec/sample', 1000000.0),
    ('100 Hz', 100),
    ('1 kHz', 1000),
    ('1 MHz', 1000000)
])
def test_process_acq(test_read_header_and_channels, units, expected):
    header = test_read_header_and_channels[0]
    channels = test_read_header_and_channels[1]
    chtrig = 2

    # set units to test that expected frequency is generated correctly
    header[1][0] = units
    phys_obj = txt.process_acq(channels, chtrig=chtrig, header=header)
    assert math.isclose(phys_obj.freq[0], expected)

Next Steps

• Clean up phys2bids/tests/data directory to remove superfluous files
• Consolidate test file downloads into one module in the testing suite and make them accessible to other tests via pytest.fixture decorators
• Use pytest.mark.parametrize more liberally throughout testing suite

---

Sorry this got so long. Nonetheless, let me know your thoughts on all this! Looking forward to discussing with everyone ✨

[smoia]

Don't be sorry, for most of us this is something totally new, and this explanation is incredibly useful!
I totally agree with all of your point, I'm glad you're already opening a PR!

About the files in test/data, they do have to be cleaned up a bit. Until now, we left a bit of everything there as a sort of backward compatibility for tests, but it's much better to have everything fetched from OSF.
I would leave the tutorial files in the folder just because it's a good example to play with phys2bids as soon as it's downloaded. I wouldn't have anything against a function that fetches them though, if it's implemented so that it would be easy to use for the average user.

[eurunuela]

Thanks a lot @rmarkello for taking the lead on this!

All your points sound very reasonable to me and will surely improve the testing suite. I also agree with @smoia that the tutorial files should stay. They are great for a first hands-on with phys2bids. Regarding the fetching function, it sounds very useful!

Let me know if there's anything I can help with!