Modify tests organization

[candleindark]

This PR re-organized some tests introduced (or modified) in #308, so that their intend is more clear.

[yarikoptic]

What is the benefit from placing into a class?

all tests in this file are for that functionality. If there is no need to group them into a class, I would prefer not to do that since not required and saves hassle while running selected tests from this file, e.g. how I did it:

python -m pytest -s -v datalad_registry/tests/test_search.py::test_search_cases

which now would need also to say again which class (although there is no other one in the file):

python -m pytest -s -v datalad_registry/tests/test_search.py::TestSearch.test_search_cases

[yarikoptic]

@candleindark I am still interested to know is there is any benefit from groupping into a class here.

[candleindark]

Here are a few benefits I can think of.

1. When all tests with the same test target are group under a class, we can make this test target obvious. It is obvious, at least to me, that test_search.py contains tests for entities defined in search.py and functionalities related to these entities. But it is not immediately obvious what which entity or functionality each test defined in test_search.py is targeted against. Case in point, the tests that are currently in test_search.py test the search functionality, but this functionality is actually not completely implemented in search.py. search.py only implements the generation of "filter criteria" to construct SQLAlchemy select statements. The target of these tests are not immediately obvious by being in test_search.py. Therefore, it can be beneficial to group all of the current tests in to a class and label/name/comment the class so that the target of these tests becomes obvious.
2. If all tests sharing the same target are grouped in a class, I can shorten the names of the tests. If, for example, I put all the current tests in a class called TestSearch, I can name the tests as test_with_invalid_query and test_with_valid_query instead of test_search_with_invalid_query and test_search_with_valid_query while conveying the same meaning to the reader. (Note: I am not using the long names in the code now because I have included these lines.
3. Grouping tests sharing the same test target into a class now make adding tests for a new target easier in the future. For example, if I want to test parse_query directly, without involvement of the database service, in the future, I can just add a new class and add new tests to it. However, if the existing tests are not in a class, I will have to put them in a class first, for organization purpose, meaning that I have to change the existing code just for organization purpose. (There may not be need to test parse_query directly. However, new entities may be defined in search.py in the future, and we may want to test those directly.)
4. Pytest also allows all tests in a class to be configured/manipulated as a group (defining use of fixture or scope, etc.). So by grouping tests in classes, you get an extra layer for configuration.

At this point, I see the applicable benefits in our case are minor since all the tests in test_search.py have the same target, and I also see the unwieldiness in navigating the tests in CLI of Pytest, which I don't use often.

[yarikoptic]

such groupping reasoning (besides potentially somewhat shorter function names, but longer together with class name anyways) IMHO stem mostly from old JUnit inspired unittest Python library. nose and even more so pytest then "broke" that pattern since it is as easy to group at the level of a Python module. With that, placing into classes, which immediately offsets most of the test module code one level to the right became IMHO generally unnecessary.

Moreover, grouppings into classes are generally less flexible than marks/tagging (could have multiple marks) in case you want to annotate tests as exhibiting some common quality (e.g. require network, db, etc). And entire modules could have marks too. And then it is possible to select tests across different files based on those marks, which would not be possible with groupping by classes.

The only potential benefit I see is that indeed in one module having multiple groups of tests having different common sets of fixtures since I am not sure if it is possible to select tests based on a fixture name they are to use, thus to simplify subselection of groups of tests within a module.

What is your take on groupping tests into classes @jwodder ?

[jwodder]

@yarikoptic I just see this as a code style thing. I personally wouldn't use test classes, but I don't think they're wrong.

[yarikoptic]

Sure. Same here and I think I never said that they are "wrong", just old-style/odd-to-encounter-these-days/unnecessary ;-)

[candleindark]

I've never paid much attention to custom marks. I have been only using the builtin ones. Thanks @yarikoptic for pointing out the benefits of having multiple marks in a test. I will utilize custom marks more in the future. Additionally, I agree with you about the cost of the additional indents. However, I still see the advantage of using classes for grouping tests sharing the same test target in a module. A custom mark usually needs registration, and it has a global nature. If all I want to do is to distinguish tests within a module according their test target, it seems to me that using a locally defined class is more immediate and natural. A custom mark seems to be more appropriate for marking tests across different modules. Additionally, an IDE like PyCharm allows selection of groups of tests by class name, not by mark, (see attached) through their GUI.

At the end, I have to say that I am biased in this matter because I use PyCharm.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (c38fdba) to head (9b4fb25).

❗ Current head 9b4fb25 differs from pull request most recent head 1bdd6a3. Consider uploading reports for the commit 1bdd6a3 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           enh-search     #319   +/-   ##
===========================================
  Coverage       98.67%   98.67%           
===========================================
  Files              52       52           
  Lines            2492     2492           
===========================================
  Hits             2459     2459           
  Misses             33       33           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.