Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify tests organization #319

Merged
merged 6 commits into from
Feb 28, 2024
Merged

Modify tests organization #319

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f2ee385
Rename tests for the overview page
candleindark Feb 28, 2024
dfaa327
Group tests for the search function into a class
candleindark Feb 28, 2024
8e85071
Rename the tests for the search function
candleindark Feb 28, 2024
130cb92
Provide doc strings for tests of search functionality
candleindark Feb 28, 2024
9b4fb25
Move tests for search out of a class
candleindark Feb 28, 2024
1bdd6a3
Specify purpose of the `test_search.py` module in comment
candleindark Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions datalad_registry/tests/test_overview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@ def test_sorting(
("datalad AND handbook", ["https://handbook.datalad.org"]),
],
)
def test_search_query(
def test_search_with_valid_query(
self, search_query: Optional[str], expected_results: list[str], flask_client
):
"""
Test for the filtering of dataset URLs in the overview page
Test searching with a valid query
"""

resp = flask_client.get("/overview/", query_string={"query": search_query})
Expand All @@ -187,9 +187,9 @@ def test_search_query(
"unknown_field:example",
],
)
def test_search_query_error(self, search_query: Optional[str], flask_client):
def test_search_with_invalid_query(self, search_query: Optional[str], flask_client):
"""
Test for the filtering of dataset URLs in the overview page
Test searching with an invalid query
"""

resp = flask_client.get("/overview/", query_string={"query": search_query})
Expand Down
196 changes: 109 additions & 87 deletions datalad_registry/tests/test_search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,6 @@
from ..search import parse_query


@pytest.mark.parametrize(
"query, exc, err",
[
("unknown_field:example", ValueError, None),
# Lark masks exceptions. We did not provide dedicated ones for all
# of them, but let's test that error message as expected
("ds_id:=example", VisitError, "Operation = is not implemented"),
# r'(haxby or halchenko) AND metadata:BIDSmetadata[bids_dataset,metalad_core]:'
# r'"BIDSVersion\": \"v"',
],
)
def test_search_errors(query, exc, err):
with pytest.raises(exc) as ce:
parse_query(query)
if err:
assert err in str(ce.value)


@pytest.fixture
def populate_with_url_metadata_for_search(
populate_with_dataset_urls, # noqa: U100 (unused argument)
Expand Down Expand Up @@ -69,72 +51,112 @@ def populate_with_url_metadata_for_search(
db.session.commit()


@pytest.mark.usefixtures("populate_with_url_metadata_for_search")
@pytest.mark.parametrize(
"query, expected",
[
# based purely on url field
("example", [1]),
("example OR handbook", [1, 3]),
# case insensitive
("Example OR handBook", [1, 3]),
("example AND handbook", []),
("example handbook", []), # implicit AND
("datalad OR handbook", [2, 3]),
("datalad AND handbook", [3]),
("datalad", [2, 3]),
("handbook", [3]),
("NOT url:handbook", [1, 2, 4]),
("NOT metadata:handbook", [1, 2, 3, 4]),
("datalad AND NOT url:handbook", [2]),
("datalad AND (NOT url:handbook)", [2]),
("datalad (NOT url:handbook)", [2]), # implicit AND
("datalad AND NOT metadata:handbook", [2, 3]),
# we get empty result ATM which fails the test. TODO - figure it out/fix!
("NOT handbook", [1, 2, 4]),
("datalad AND (NOT handbook)", [2]),
("handbook datalad", [3]), # should be identical result to above AND
("handbook url:datalad", [3]),
("handbook url:?datalad", [3]), # identical to above
("handbook ds_id:datalad", []),
("handbook OR ds_id:datalad", [3]),
("url:handbook OR ds_id:datalad", [3]),
("url:handbook OR ds_id:844c", [1, 2, 3]),
("(url:handbook OR metadata[metalad_core]:meta1value) AND ds_id:844c", [2, 3]),
(
"(url:?handbook OR metadata[metalad_core]:?meta1value) AND ds_id:?844c",
[2, 3],
),
("(url:handbook OR metadata[metalad_core]:meta3value) AND ds_id:844C", [1, 3]),
("(url:handbook OR metadata[metalad_core]:value) AND ds_id:844c", [1, 2, 3]),
(
"(url:handbook OR metadata[metalad_core]:value) ds_id:844c",
[1, 2, 3],
), # implicit AND
("meta2value", [2]),
# search within a JSON record. Space between key and value would matter
(r'metadata:"meta1\": \"meta1value\""', [2]),
('metadata:"value"', [1, 2]),
('metadata[metalad_studyminimeta]:"value"', []),
# ATM only exact match for extractor
('metadata[metalad]:"value"', []),
('metadata[metalad_core]:"value"', [1, 2]),
# OR among multiple listed, ok to have unknown
('metadata[metalad_core,metalad_studyminimeta,unknown]:"value"', [1, 2]),
# Prototypical query for which we do not have full support yet, e.g.
# regex matching :~
# (r"""((jim AND NOT haxby AND "important\" paper") OR ds_id:~"^000[3-9]..$"
# OR url:"example.com") AND metadata:non AND metadata[ex1,ex2]:"specific data"
# AND metadata[extractor2]:data""", []),
# Find datasets with the last
# (metadata[bids_dataset][Authors][-1]:haxby ...)
],
)
def test_search_cases(flask_app, query, expected):
r = parse_query(query)
# print(f"QUERY {query}: {r}")
with flask_app.app_context():
result = db.session.execute(select(RepoUrl).filter(r))
hits = [_.id for _ in result.scalars().all()]
# print(expected, hits)
assert hits == expected
class TestSearch:
"""
Tests for the search functionality
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@candleindark candleindark Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2024-03-01 at 3 22 50 PM


@pytest.mark.parametrize(
"query, exc, err",
[
("unknown_field:example", ValueError, None),
# Lark masks exceptions. We did not provide dedicated ones for all
# of them, but let's test that error message as expected
("ds_id:=example", VisitError, "Operation = is not implemented"),
# r'(haxby or halchenko) AND '
# r'metadata:BIDSmetadata[bids_dataset,metalad_core]:'
# r'"BIDSVersion\": \"v"',
],
)
def test_with_invalid_query(self, query, exc, err):
"""
Test the search functionality in handling invalid queries
"""
with pytest.raises(exc) as ce:
parse_query(query)
if err:
assert err in str(ce.value)

@pytest.mark.usefixtures("populate_with_url_metadata_for_search")
@pytest.mark.parametrize(
"query, expected",
[
# based purely on url field
("example", [1]),
("example OR handbook", [1, 3]),
# case insensitive
("Example OR handBook", [1, 3]),
("example AND handbook", []),
("example handbook", []), # implicit AND
("datalad OR handbook", [2, 3]),
("datalad AND handbook", [3]),
("datalad", [2, 3]),
("handbook", [3]),
("NOT url:handbook", [1, 2, 4]),
("NOT metadata:handbook", [1, 2, 3, 4]),
("datalad AND NOT url:handbook", [2]),
("datalad AND (NOT url:handbook)", [2]),
("datalad (NOT url:handbook)", [2]), # implicit AND
("datalad AND NOT metadata:handbook", [2, 3]),
# we get empty result ATM which fails the test. TODO - figure it out/fix!
("NOT handbook", [1, 2, 4]),
("datalad AND (NOT handbook)", [2]),
("handbook datalad", [3]), # should be identical result to above AND
("handbook url:datalad", [3]),
("handbook url:?datalad", [3]), # identical to above
("handbook ds_id:datalad", []),
("handbook OR ds_id:datalad", [3]),
("url:handbook OR ds_id:datalad", [3]),
("url:handbook OR ds_id:844c", [1, 2, 3]),
(
"(url:handbook OR metadata[metalad_core]:meta1value) AND ds_id:844c",
[2, 3],
),
(
"(url:?handbook OR metadata[metalad_core]:?meta1value) AND ds_id:?844c",
[2, 3],
),
(
"(url:handbook OR metadata[metalad_core]:meta3value) AND ds_id:844C",
[1, 3],
),
(
"(url:handbook OR metadata[metalad_core]:value) AND ds_id:844c",
[1, 2, 3],
),
(
"(url:handbook OR metadata[metalad_core]:value) ds_id:844c",
[1, 2, 3],
), # implicit AND
("meta2value", [2]),
# search within a JSON record. Space between key and value would matter
(r'metadata:"meta1\": \"meta1value\""', [2]),
('metadata:"value"', [1, 2]),
('metadata[metalad_studyminimeta]:"value"', []),
# ATM only exact match for extractor
('metadata[metalad]:"value"', []),
('metadata[metalad_core]:"value"', [1, 2]),
# OR among multiple listed, ok to have unknown
('metadata[metalad_core,metalad_studyminimeta,unknown]:"value"', [1, 2]),
# Prototypical query for which we do not have full support yet, e.g.
# regex matching :~
# (r"""((jim AND NOT haxby AND "important\" paper") OR
# ds_id:~"^000[3-9]..$"
# OR url:"example.com") AND metadata:non
# AND metadata[ex1,ex2]:"specific data"
# AND metadata[extractor2]:data""", []),
# Find datasets with the last
# (metadata[bids_dataset][Authors][-1]:haxby ...)
],
)
def test_with_valid_query(self, flask_app, query, expected):
"""
Test the search functionality in handling valid queries
"""
r = parse_query(query)
# print(f"QUERY {query}: {r}")
with flask_app.app_context():
result = db.session.execute(select(RepoUrl).filter(r))
hits = [_.id for _ in result.scalars().all()]
# print(expected, hits)
assert hits == expected