OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
95 stars 73 forks source link

Rethink and upgrade testing scheme #552

Closed leewujung closed 2 years ago

leewujung commented 2 years ago

In our current testing setup, when PR to dev the test is only run on the modified module and not on the downstream tests. The entire set of tests are run when the PR is merged to dev (or main). So what we have right now is a little brittle even though bugs would be caught and fixed eventually. It'll be good for us to rethink how we can upgrade this and still with reasonable the CI running time.

Below are conversions from discussions re testing in #547:

@lsetiawan :

In practice, most software CI actually runs everything anyways to make sure your changes doesn't break anything, we just decided to make this custom, brittle, intelligence 😜

@leewujung :

In theory we can get out of the current brittle setup once most big test data files are updated to a smaller size. I updated 2 major ones, but ran out of time to do the last couple files before I had to return a hardware dongle needed to generate data we test against -- the next time I can do this is likely late March - April.

BUT regardless, it is unlikely we'll be able to reduce all files to be KB size like the new EK80 test files I collected, since we don't have access to many instrument variations that were used to collect the diverse test files.

Also related on the current status: @lsetiawan :

as long as the isolated module test works you can merge to dev for a full test. Or you can always just use the Needs complete testing label with the PR if you know that particular function touches many different things

@emiliom :

And whenever you have a need for that mechanism but don't remember how to use it, go to the docs: https://echopype.readthedocs.io/en/stable/contributing.html#continuous-integration-github-actions

lsetiawan commented 2 years ago

I think the solution may be speeding up the test and like you said in #553, run all tests every time. We have not utilized the parallel testing... I know there were some issues there, but I'll get back to it.

leewujung commented 2 years ago

From @lsetiawan in #601:

Thinking about it, I wonder if we can add like specific connected modules that should also be run for that module if it uses functions from those connected modules. Does that make sense? 😆 This way, we can have an "extensive" test without having to run the whole test!

leewujung commented 2 years ago

Noted down some 3.9 and 3.10 test failures for investigation: https://github.com/OSOceanAcoustics/echopype/actions/runs/2153214186/attempts/1 Similar test failures have occurred a few times in the v0.5.6-->v0.6.0 dev period.

leewujung commented 2 years ago

Let's close this one and open a new issue on whether we want to switch to always running all tests regardless of which files are changed in a PR.

leewujung commented 2 years ago

Seems like we can close this now that @lsetiawan has put in quite a bit of work to make the tests more robust?

lsetiawan commented 2 years ago

Sounds good to me. Thanks