Test Refactor

[ericspod]

The tests directory has gotten large over time as we've added new components with their test cases. It's absolutely a good thing to have thorough testing but as a almost-flat directory it's hard to find things and it's cumbersome to view so many files in IDEs. Further there are likely many areas of refactoring that we can do to reduce duplicate code and introduce more helper routines to do common tasks. I would suggest a few things to improve our tests:

• Break the contents of tests into subdirectories mirroring those in the monai directory. Tests for transforms would go under transforms, those for networks under networks, etc. It may be necessary to have more directory structure under these as well but this doesn't need to be overcomplicated.
• Common patterns and ways of generating test data can be factored out into utils.
• A common pattern is to generate test cases for use with parameterized in deeply-nested for loops, eg.:

TEST_CASES=[]
for arg1 in [2,4]:
    for arg2 in [8,16]:
        TEST_CASES.append([{"arg1": arg1, "arg2": arg2}, arg1, arg2])

A simple routine for doing product over dictionaries can reduce this code significantly:

def dict_product(**items):  # should be put in core utils somewhere
    keys=items.keys()
    values=items.values()
    for pvalues in product(*values):
        yield dict(zip(keys, pvalues))
...
TEST_CASES=[[d, d["arg1"], d["arg2"]] for d in dict_product(arg1=[2,4], arg2=[8,16])]

• A number of tests use large data items or perform quite a few iterations of training. These are slow running so it would be good to go through the slower tests to see if any speedup can be done without losing testing value.
• Similarly how synthetic data is created should be re-evaluated to see what commonality can be factored out and standardised.
• Many tests are silent while others have output whether they pass or not. My feeling was always that tests should be silent unless they fail then they should be loud about why. This keeps the clutter down so it's clear when tests are passing, and when they don't their output is easier to spot and understand. This is maybe personal taste but should we rethink this behaviour?

[garciadias]

Hi there. I am interested in contributing, and this issue is something I would be comfortable helping with. Following the contribution guide, I am communicating. 🙃
Should I open a pull request before starting or as soon I have a done a little work?

[ericspod]

Hi @garciadias thanks for offering to help. Did you want to do one aspect of what I was proposing or more? I would suggest choosing which one you want to do first, consider how much work that would be, then starting implementing some things on your own. I would suggest waiting until you have something to review before raising the PR, unless you want help at some point in which case you can open a draft PR so we can look at your code. All these changes are important in my mind somewhat equally so I'll leave it to you to pick something to start with, let me know what you decide and we can discuss from there.

[garciadias]

Hi @ericspod,

Thank you for your answer.

I was thinking about starting with this one:

• A number of tests use large data items or perform quite a few iterations of training. These are slow running so it would be good to go through the slower tests to see if any speedup can be done without losing testing value.

I see the -q option skips the tests flagged as skip_if_quick, so my approach would be to look into the tests with this flag to understand which are actual integration tests and which could mock heavy functions. My main goal would be to increase the coverage with the -q option so you minimise the amount of slow tests that need to be run, making the development process more smooth.

Hopefully, this would give me enough knowledge of the test base so I could proceed with the first point:

• Break the contents of tests into subdirectories mirroring those in the monai directory. Tests for transforms would go under transforms, those for networks under networks, etc. It may be necessary to have more directory structure under these as well but this doesn't need to be overcomplicated.

What do you think?

[ericspod]

Hi @ericspod,

Thank you for your answer.

I was thinking about starting with this one:

• A number of tests use large data items or perform quite a few iterations of training. These are slow running so it would be good to go through the slower tests to see if any speedup can be done without losing testing value.

I see the -q option skips the tests flagged as skip_if_quick, so my approach would be to look into the tests with this flag to understand which are actual integration tests and which could mock heavy functions. My main goal would be to increase the coverage with the -q option so you minimise the amount of slow tests that need to be run, making the development process more smooth.

Hopefully, this would give me enough knowledge of the test base so I could proceed with the first point:

• Break the contents of tests into subdirectories mirroring those in the monai directory. Tests for transforms would go under transforms, those for networks under networks, etc. It may be necessary to have more directory structure under these as well but this doesn't need to be overcomplicated.

What do you think?

That sounds like a good plan so go for it. My feeling with the slow tests is that they might be using data that's much larger than needed for the same testing value, as well as running for too long. Testing things for functionality and accuracy are two different tasks and I wonder if there's too much focus in places on getting the right result. Definitely have a look around and see what you come up with, you can also make a new issue if you like detailing your findings if more discussion would help.

[garciadias]

Hi @KumoLiu,

Thank you for all the help from you and @ericspod. I am so happy that my PR was merged.
Unfortunately, I think my PR does not solve this issue entirely. It may be worth breaking it into multiple issues, but I just want to highlight that I have solved only the first bullet point of the six proposed by Eric.

[ericspod]

Hi @KumoLiu,

Thank you for all the help from you and @ericspod. I am so happy that my PR was merged. Unfortunately, I think my PR does not solve this issue entirely. It may be worth breaking it into multiple issues, but I just want to highlight that I have solved only the first bullet point of the six proposed by Eric.

Thanks for the work so far, I've reopened this issue and we can continue on the other points. I think there's still some moving around that we should do in the tests directory but it'll be much easier now knowing that most of the changes are done and coordinated with blossom.

[garciadias]

Great! Let's discuss how we can move forward on this together so we don't overlap efforts. Also, I will try to keep my PRs much shorter this time. One strategy may be to work on these by refactoring one subfolder at a PR.