Renamed test functions in test_lowest_common_ancestors #6110
Conversation
There was a problem hiding this comment.
Thanks @tindi-plus this is the right general idea, though it'd be nice to preserve the function that is being tested in the test name (or at least something similar - I arbitrarily suggested abbreviating "lca"): see the suggestions below.
Of course, in this case the tests are implemented as methods under the TestTreeLCA class, so repeating the function name in the test is arguably a bit redundant.
| @@ -51,38 +51,38 @@ def assert_has_same_pairs(d1, d2): | |||
| for (a, b) in ((min(pair), max(pair)) for pair in chain(d1, d2)): | |||
| assert get_pair(d1, a, b) == get_pair(d2, a, b) | |||
|
|
|||
| def test_tree_all_pairs_lowest_common_ancestor1(self): | |||
| def test_specifying_root_optional_in_lca(self): | |||
There was a problem hiding this comment.
| def test_specifying_root_optional_in_lca(self): | |
| def test_tree_all_pairs_lca_defaults(self): |
There was a problem hiding this comment.
I have started work on the changes and a quick question:
Since the test method is actually testing that adding a starting root produces the same result as not adding a root, won't this be more descriptive?
| def test_specifying_root_optional_in_lca(self): | |
| def test_tree_all_pairs_lca_root_is_optional(self): |
There was a problem hiding this comment.
I'd probably call it lca_default_root, but honestly it's quite subjective!
Remember - the goal is that the test names are informative so that when they fail there is some indication of what the test pertains to. A useful exercise to evaluate this is to run pytest with the verbose option, e.g. pytest -v networkx/algorithms/tests/test_lowest_common_ancestors.py
|
Thank you @rossbar for the suggestions. Working on it right away. |
|
Hello @rossbar, I think I need help with suitable names on lines 157 and 275. Thank you. |
There was a problem hiding this comment.
Generally I think the renaming here is good, thanks @tindi-plus .
One final piece of the cleanup would be to remove the docstrings which are redundant. Again, this is another subjective thing so there is no right answer, but many of the test descriptions in the docstrings are redundant now that the test names are more accurate.
|
Hello @rossbar, suggestions implemented. Redundant docstrings removed. |
There was a problem hiding this comment.
Thanks @tindi-plus I took the liberty of pushing up just a few final suggestions, but this LGTM!
|
Thank you @rossbar for the entire engagement. |
* Renamed test functions in test_lowest_common_ancestors * Updated test method names. * Removed redundant docstrings * Minor touchups. Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
* Renamed test functions in test_lowest_common_ancestors * Updated test method names. * Removed redundant docstrings * Minor touchups. Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
* Renamed test functions in test_lowest_common_ancestors * Updated test method names. * Removed redundant docstrings * Minor touchups. Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
This pull request will fix issue #5863.
The method names of the test classes have been modified to depict what the method is testing.
The modifications are for networkx/algorithms/tests/test_lowest_common_ancestors.py