Tests for the image_io sub-module

[K-Meech]

Closes #44

Description

This PR adds further tests for the image_io sub-module, bringing coverage for each file to:

• save 100%
• load 99%
• utils 91%

It also updates some minor parts of the image_io docstrings

[alessandrofelder]

Looking good - thanks @K-Meech

Some tiny comments/questions (because I am curious) for you to consider before merging.

[alessandrofelder]

Is it worth to briefly mention a typical use case for that text file?
Reading this, I understood what was going on, but wasn't sure why.

[Edit:] after reviewing the rest of the PR, my understanding as to why we need this is that we support this input format, but never write to it, so we want to have this to test. Presumably this format is useful for parallel/lazy IO? Could this be a fixture just for consistency - so all our test code is fixtures or test functions?

[K-Meech]

Yes that's right - it's needed as we support reading from a txt file with load_img_sequence, but there's no corresponding save function. I'm not sure what the use-case is for such a text file - I guess if you have tiff files spread over multiple directories it could be useful?

I'll look at changing this into a fixture now

[K-Meech]

@alessandrofelder - I'm trying to convert this to a fixture, but having problems with allowing it to be optionally shuffled like here. Is there a simple way to pass a parameter like shuffle=True to a fixture? Otherwise, I could have two fixtures - one for a shuffled text file and one for not, but they will have pretty much identical code in both unless I refactor it out into some kind of helper function. Any ideas?

[willGraham01]

Fixtures can't take optional arguments, but can be indirectly parameterised:

import pytest

@pytest.fixture
def my_fixture(msg: str, reps: int) -> str:
    return msg * reps


@pytest.mark.parametrize(
    "msg, reps, expected",
    [
        pytest.param("Hi", 2, "HiHi"),
        pytest.param("Hi", 1, "Hi"),
    ],
)
def test_thing(my_fixture, expected):
    assert my_fixture == expected

will pass, for example. However if you change reps: int to reps: int = 2 in the fixture definition, the invoking pytest will fail.

However I don't think write_tiff_sequence_with_txt_file can be made into a fixture (depending on how you're using it). I'm prepared to be proven wrong though 😅 But in my understanding, fixtures have to either:

• Return an object that can be constructed a-priori to the test running.
• Be a sequence of executable steps that serves as either the setup or teardown of a test.

Here, write_tiff_sequence_with_txt_file needs to be run in the middle of a test, so it has to be a function, and can't be a fixture. You could try redesigning it as a setup fixture though - assuming you're only ever using it at the start of a test, before anything else runs:

@pytest.fixture
def write_tiff_sequence_with_txt_file(txt_file, input_array, shuffle):
    # Do things prior to running a test
    yield # This is where the test will run


@pytest.mark.usefixtures("write_tiff_sequency_with_txt_file")
@pytest.mark.parametrize(
    "txt_file, input_array, shuffle, other_args_used_by_test",
    [
        ["foo.txt", [1,2,3], True, "some_fancy_args"],
    ],
)
def test_things(txt_file, input_array, shuffle, other_args_used_by_test) -> None:
    # write_tiff_sequence_with_txt_file will run here before the actual test instructions
    assert other_args_used_by_test
    # Note that txt_file, input_array, shuffle don't actually have to be in the test-function declaration,
    # test_things(other_args_used_by_test) also works fine.
    # But if you DO include them, the values of the variables will be usable within the test-function scope too.
    # IE the line
    assert (
        txt_file is not None
    )  
    # Only works if test_things(txt_file, other_args_used_by_test) is used as the definition, 
    # but not if test_things(other_args_used_by_test) is used instead.

Otherwise, I don't think it can be "fixture-ised".

[K-Meech]

Thanks for the info @willGraham01! That makes it a lot clearer.
I think I could redesign it as a setup fixture, but this would mean I'd need a separate test case for txt files with load_any, rather than having it as a parameter here

As we support loading txt files in image_io via load_any and load_img_sequence, perhaps it would make sense for this to be moved into image_io to allow saving this kind of file also? Then the test functions could simply call this?

[alessandrofelder]

Interesting. Yea fixture parameters are tricky!

In this specific case, IIUC we need the shuffle=True part for testing the sort argument of load_img_sequence only, whereas we will need the shuffle=False case in the source code (as part of addressing #50). I don't think we'd ever want to intentionally shuffle the order of paths in a text file in our source code?

So I'd suggest

• keeping write_tiff_sequence_with_txt_file not a fixture
• splitting the shuffle part into a fixture that overwrites the textfile with a shuffled version of it.
• as part of Add a save_any function to image_io #50, move write_tiff_sequence_with_txt_file to the source code

Is that sensible?

[K-Meech]

Sounds good to me! I'll add these points to the issue + split the shuffle part now

[K-Meech]

@alessandrofelder I split shuffle into a fixture now. Could you take a look + merge if you're happy with it? Then I'll work on #50