[ENH] Refactor pick connection tests

[gtdang]

This is a refactoring of the pick_condition tests. I moved them out of the test_network.py file and into its own file. This function need many tests because it is very flexible and can take many different types of input arguments.

The easiest way to review the changes is commit-by-commit. I paired each group of new tests with the deletion of corollary tests in the old file. - Note the corollary deletion for the list of strings test in (https://github.com/jonescompneurolab/hnn-core/pull/745/commits/06036214ec782fdeaa2e9bcfeaae1452caef345c) suck its way into the preceding commit 3472031).

• The tests were split into individual tests by functionality
• Tests are organized under the TestPickConnection class
• pytest fixtures and parameterization are used where possible

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.62%. Comparing base (54bd278) to head (4b018cc). Report is 34 commits behind head on master.

:exclamation: Current head 4b018cc differs from pull request most recent head eee0d05. Consider uploading reports for the commit eee0d05 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #745 +/- ## ======================================= Coverage 92.62% 92.62% ======================================= Files 27 27 Lines 4919 4919 ======================================= Hits 4556 4556 Misses 363 363 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jasmainak]

not super convinced about the refactoring ... I'm finding it less readable. Obviously, it uses pytest.mark.parametrize which is nice but the flip-side is that the pick_connection test is divided across multiple functions. Note that a test_xxx.py file exists only if there is xxx.py file. In other words, we would need a pick_connections.py file for a test_pick_connections.py file. Adding a separate test file for a single function would mess with this logic.

Obviously the original tests were not clear enough to you as a new contributor, so there is room for improvement. Perhaps you can discuss with @ntolley how to do this best as he is the one who first wrote these tests.

[gtdang]

I can understand that this change may look a bit difficult to follow at first. However in the long-run refactoring the tests in the codebase will make development and bug-tracking much easier for developers and maintainers. The essence of a unit test is to test the smallest unit of functionality in isolation (without dependencies on other functions). This allows a way to quickly determine what part of the a code base is impacted by a breaking change when a developer is adding new features or quashing bugs.

The old version of the test_network.test_network_connectivity is not isolated (see below). It is a nearly 300 line test that vaguely tests different functionalities. You have to study it hard to realize that it is actually several independent tests for the NetworkBuilder class, add_connection method, pick_connection function, and the and clear_connectivity method (I think in that same order). At the very least each of these should be their own tests.

https://github.com/jonescompneurolab/hnn-core/blob/d28ddb75fb9c2f12c5fda100efbfdf1c66261d9c/hnn_core/tests/test_network.py#L533-L829

Why is this a bad practice?

• The tests for a subsequent function are dependent on functionality of preceding unrelated functions. If there is a problem with NetworkBuilder AND pick_connection, I'm not going to find that out until the NetworkBuilder problem is addressed and the test is re-run.
• It takes longer to debug. With the old function you get a single fail and must debug which line it crashed on to figure out what the error was. In the refactor you just need to look at the test name and doc-string to figure out the broken functionality. Let's take for an example if test_pc_1arg_str and test_pc_1arg_list_str both failed but all other tests passed? There's likely an issue with string processing in the function.

Now as to why pick_connection should be broken up to smaller tests?

• This function was written to be very flexible. There are many optional arguments and each argument can take different data types (int, string, list, range). Its flexibility is useful but difficult to debug when something goes wrong. Isolating the different input arguments and error behaviors to individual tests helps to quickly pin-point specific functionalities that broke but also showing which functionalities still work. When you run pytest on the module it will run all the tests, not just stopping at the first fail.

The pick_connection tests doesn't need to be in its own file--it could be added back to test_network. However I figured the difference in test style would be confusing to marry. Ideally the test_network module and other loosely-scoped tests would be refactored in a similar fashion and pick-connection can be re-combined. Until then the separate file is an easier reference for writing clean unit tests.

[jasmainak]

no disagreement that test_network_connectivity is a beast and needs to be simplified.

I like the idea of separate functions to test add_connection, pick_connection etc. But what you've done here is broken the test for pick_connection into multiple functions with cryptic names ... I would suggest starting with a basic refactor first -- separating out tests for pick_connection, add_connection into its own function while keeping them in test_network.

General comment: any changes you make to the repo cannot be contingent on future fixes that may or may not happen ... the main branch should be usable by users and its only purpose is to catch bugs early before release. It is not an "experimental" branch.

[gtdang]

In the latest refactor I've pulled the tests into test_network.py. I grouped the individual tests into a class TestPickConnection so they are grouped with the function they are testing. The names are more explicitly spelled out. Scipy often uses this organization into classes when testing different functionalities of a function.

[jasmainak]

Thanks for the refactor! Are all the tests that were previously covered also included in the new tests? It wasn't immediately obvious to me. Also can you point me to the scipy tests which follow the similar format for my curiosity.

[gtdang]

Thanks for the refactor! Are all the tests that were previously covered also included in the new tests? It wasn't immediately obvious to me. Also can you point me to the scipy tests which follow the similar format for my curiosity.

Sorry for the late reply! Yes all tests are covered.

Here's one example from Scipy: test_binned_statistic.py

Also, here's the pytest documentation about grouping tests with classes.

[ntolley]

Great work @gtdang !!

Apologies on the slow review, wanted to make sure I had time to double check how the tests were re-implemented in the refactor.