[TST] Refactor tests (#269)

* refactor all mock_get variants into fixtures

* test API response for aggregate cohort query

* add docstring for new test

* rename factory fixtures and update docstrings for clarity

tests/conftest.py

@@ -53,7 +53,7 @@ def test_data():
 
 @pytest.fixture
 def mock_post_query_to_graph():
-    """Mock post_query_to_graph function that returns toy data containing a dataset with no modalities for testing."""
+    """Mock post_query_to_graph function that returns toy aggregate data containing a dataset with no modalities for testing."""
 
     def mockreturn(query, timeout=5.0):
         return {
@@ -104,10 +104,32 @@ def mockreturn(query, timeout=5.0):
 
 
 @pytest.fixture
-def mock_successful_get(test_data):
-    """Mock get function that returns non-empty query results."""
+def mock_query_matching_dataset_sizes():
+    """
+    Mock query_matching_dataset_sizes function that returns the total number of subjects for a toy dataset 12345.
+    Can be used together with mock_post_query_to_graph to mock both the POST step of a cohort query and the corresponding query for dataset size,
+    in order to test how the response from the graph is processed by the API (crud.get).
+    """
+
+    def _mock_query_matching_dataset_sizes(dataset_uuids):
+        return {"http://neurobagel.org/vocab/12345": 200}
+
+    return _mock_query_matching_dataset_sizes
+
+
+@pytest.fixture
+def mock_get_with_exception(request):
+    """
+    Mock get function that raises a specified exception.
+
+    A parameter passed to this fixture via indirect parametrization is received by the internal factory function before it is passed to a test.
+
+    Example usage in test function:
+        @pytest.mark.parametrize("mock_get_with_exception", [HTTPException(500)], indirect=True)
+        (this tells mock_get_with_exception to raise an HTTPException)
+    """
 
-    async def mockreturn(
+    async def _mock_get_with_exception(
         min_age,
         max_age,
         sex,
@@ -118,16 +140,24 @@ async def mockreturn(
         assessment,
         image_modal,
     ):
-        return test_data
+        raise request.param
 
-    return mockreturn
+    return _mock_get_with_exception
 
 
 @pytest.fixture
-def mock_invalid_get():
-    """Mock get function that does not return any response (for testing invalid parameter values)."""
+def mock_get(request):
+    """
+    Mock get function that returns an arbitrary response or value (can be None). Can be used to testing error handling of bad requests.
+
+    A parameter passed to this fixture via indirect parametrization is received by the internal factory function before it is passed to a test.
+
+    Example usage in test function:
+        @pytest.mark.parametrize("mock_get", [None], indirect=True)
+        (this tells mock_get to return None)
+    """
 
-    async def mockreturn(
+    async def _mock_get(
         min_age,
         max_age,
         sex,
@@ -138,9 +168,29 @@ async def mockreturn(
         assessment,
         image_modal,
     ):
-        return None
+        return request.param
+
+    return _mock_get
 
-    return mockreturn
+
+@pytest.fixture
+def mock_successful_get(test_data):
+    """Mock get function that returns non-empty, valid aggregate query result data."""
+
+    async def _mock_successful_get(
+        min_age,
+        max_age,
+        sex,
+        diagnosis,
+        is_control,
+        min_num_imaging_sessions,
+        min_num_phenotypic_sessions,
+        assessment,
+        image_modal,
+    ):
+        return test_data
+
+    return _mock_successful_get
 
 
 @pytest.fixture()
@@ -160,7 +210,7 @@ def terms_test_data():
 def mock_successful_get_terms(terms_test_data):
     """Mock get_terms function that returns non-empty results."""
 
-    async def mockreturn(data_element_URI, term_labels_path):
+    async def _mock_successful_get_terms(data_element_URI, term_labels_path):
         return terms_test_data
 
-    return mockreturn
+    return _mock_successful_get_terms

tests/test_query.py

@@ -3,6 +3,7 @@
 import pytest
 from fastapi import HTTPException
 
+import app.api.utility as util
 from app.api import crud
 
 
@@ -53,15 +54,13 @@ def mock_post_query_to_graph(query, timeout=5.0):
 
 
 def test_null_modalities(
-    test_app, test_data, mock_post_query_to_graph, monkeypatch
+    test_app,
+    mock_post_query_to_graph,
+    mock_query_matching_dataset_sizes,
+    monkeypatch,
 ):
     """Given a response containing a dataset with no recorded modalities, returns an empty list for the imaging modalities."""
 
-    def mock_query_matching_dataset_sizes(dataset_uuids):
-        return {
-            "http://neurobagel.org/vocab/12345": 200,
-        }
-
     monkeypatch.setattr(crud, "post_query_to_graph", mock_post_query_to_graph)
     monkeypatch.setattr(
         crud, "query_matching_dataset_sizes", mock_query_matching_dataset_sizes
@@ -114,6 +113,7 @@ def test_get_valid_age_single_bound(
     assert response.json() != []
 
 
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
 @pytest.mark.parametrize(
     "invalid_min_age, invalid_max_age",
     [
@@ -123,11 +123,11 @@ def test_get_valid_age_single_bound(
     ],
 )
 def test_get_invalid_age(
-    test_app, mock_invalid_get, invalid_min_age, invalid_max_age, monkeypatch
+    test_app, mock_get, invalid_min_age, invalid_max_age, monkeypatch
 ):
     """Given an invalid age range, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
         f"/query/?min_age={invalid_min_age}&max_age={invalid_max_age}"
     )
@@ -147,10 +147,11 @@ def test_get_valid_sex(test_app, mock_successful_get, valid_sex, monkeypatch):
     assert response.json() != []
 
 
-def test_get_invalid_sex(test_app, mock_invalid_get, monkeypatch):
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
+def test_get_invalid_sex(test_app, mock_get, monkeypatch):
     """Given an invalid sex string, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get("/query/?sex=apple")
     assert response.status_code == 422
 
@@ -169,15 +170,16 @@ def test_get_valid_diagnosis(
     assert response.json() != []
 
 
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
 @pytest.mark.parametrize(
     "invalid_diagnosis", ["sn0med:35489007", "apple", ":123456"]
 )
 def test_get_invalid_diagnosis(
-    test_app, mock_invalid_get, invalid_diagnosis, monkeypatch
+    test_app, mock_get, invalid_diagnosis, monkeypatch
 ):
     """Given an invalid diagnosis, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(f"/query/?diagnosis={invalid_diagnosis}")
     assert response.status_code == 422
 
@@ -194,20 +196,20 @@ def test_get_valid_iscontrol(
     assert response.json() != []
 
 
-def test_get_invalid_iscontrol(test_app, mock_invalid_get, monkeypatch):
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
+def test_get_invalid_iscontrol(test_app, mock_get, monkeypatch):
     """Given a non-boolean is_control value, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get("/query/?is_control=apple")
     assert response.status_code == 422
 
 
-def test_get_invalid_control_diagnosis_pair(
-    test_app, mock_invalid_get, monkeypatch
-):
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
+def test_get_invalid_control_diagnosis_pair(test_app, mock_get, monkeypatch):
     """Given a non-default diagnosis value and is_control value of True, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
         "/query/?diagnosis=snomed:35489007&is_control=True"
     )
@@ -241,21 +243,22 @@ def test_get_valid_min_num_sessions(
     assert response.json() != []
 
 
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
 @pytest.mark.parametrize(
     "session_param",
     ["min_num_phenotypic_sessions", "min_num_imaging_sessions"],
 )
 @pytest.mark.parametrize("invalid_min_num_sessions", [-3, 2.5, "apple"])
 def test_get_invalid_min_num_sessions(
     test_app,
-    mock_invalid_get,
+    mock_get,
     session_param,
     invalid_min_num_sessions,
     monkeypatch,
 ):
     """Given an invalid minimum number of imaging sessions, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
         f"/query/?{session_param}={invalid_min_num_sessions}"
     )
@@ -271,15 +274,16 @@ def test_get_valid_assessment(test_app, mock_successful_get, monkeypatch):
     assert response.json() != []
 
 
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
 @pytest.mark.parametrize(
     "invalid_assessment", ["bg01:cogAtlas-1234", "cogAtlas-1234"]
 )
 def test_get_invalid_assessment(
-    test_app, mock_invalid_get, invalid_assessment, monkeypatch
+    test_app, mock_get, invalid_assessment, monkeypatch
 ):
     """Given an invalid assessment, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(f"/query/?assessment={invalid_assessment}")
     assert response.status_code == 422
 
@@ -307,28 +311,16 @@ def test_get_valid_available_image_modal(
     assert response.json() != []
 
 
+@pytest.mark.parametrize("mock_get", [[]], indirect=True)
 @pytest.mark.parametrize(
     "valid_unavailable_image_modal",
     ["nidm:Flair", "owl:sameAs", "nb:FlowWeighted", "snomed:something"],
 )
 def test_get_valid_unavailable_image_modal(
-    test_app, valid_unavailable_image_modal, monkeypatch
+    test_app, valid_unavailable_image_modal, mock_get, monkeypatch
 ):
     """Given a valid, pre-defined, and unavailable image modality, returns a 200 status code and an empty list of results."""
 
-    async def mock_get(
-        min_age,
-        max_age,
-        sex,
-        diagnosis,
-        is_control,
-        min_num_imaging_sessions,
-        min_num_phenotypic_sessions,
-        assessment,
-        image_modal,
-    ):
-        return []
-
     monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(
         f"/query/?image_modal={valid_unavailable_image_modal}"
@@ -338,43 +330,57 @@ async def mock_get(
     assert response.json() == []
 
 
+@pytest.mark.parametrize("mock_get", [None], indirect=True)
 @pytest.mark.parametrize(
     "invalid_image_modal", ["2nim:EEG", "apple", "some_thing:cool"]
 )
 def test_get_invalid_image_modal(
-    test_app, mock_invalid_get, invalid_image_modal, monkeypatch
+    test_app, mock_get, invalid_image_modal, monkeypatch
 ):
     """Given an invalid image modality, returns a 422 status code."""
 
-    monkeypatch.setattr(crud, "get", mock_invalid_get)
+    monkeypatch.setattr(crud, "get", mock_get)
     response = test_app.get(f"/query/?image_modal={invalid_image_modal}")
     assert response.status_code == 422
 
 
+@pytest.mark.parametrize(
+    "mock_get_with_exception", [HTTPException(500)], indirect=True
+)
 @pytest.mark.parametrize(
     "undefined_prefix_image_modal",
     ["dbo:abstract", "sex:apple", "something:cool"],
 )
 def test_get_undefined_prefix_image_modal(
-    test_app, undefined_prefix_image_modal, monkeypatch
+    test_app,
+    undefined_prefix_image_modal,
+    mock_get_with_exception,
+    monkeypatch,
 ):
     """Given a valid and undefined prefix image modality, returns a 500 status code."""
 
-    async def mock_get(
-        min_age,
-        max_age,
-        sex,
-        diagnosis,
-        is_control,
-        min_num_imaging_sessions,
-        min_num_phenotypic_sessions,
-        assessment,
-        image_modal,
-    ):
-        raise HTTPException(500)
-
-    monkeypatch.setattr(crud, "get", mock_get)
+    monkeypatch.setattr(crud, "get", mock_get_with_exception)
     response = test_app.get(
         f"/query/?image_modal={undefined_prefix_image_modal}"
     )
     assert response.status_code == 500
+
+
+def test_aggregate_query_response_structure(
+    test_app,
+    set_test_credentials,
+    mock_post_query_to_graph,
+    mock_query_matching_dataset_sizes,
+    monkeypatch,
+):
+    """Test that when aggregate results are enabled, a cohort query response has the expected structure."""
+    monkeypatch.setenv(util.RETURN_AGG.name, "true")
+    monkeypatch.setattr(crud, "post_query_to_graph", mock_post_query_to_graph)
+    monkeypatch.setattr(
+        crud, "query_matching_dataset_sizes", mock_query_matching_dataset_sizes
+    )
+
+    response = test_app.get("/query/")
+    assert all(
+        dataset["subject_data"] == "protected" for dataset in response.json()
+    )