diff --git a/tests/conftest.py b/tests/conftest.py index 05c19c2..198b97c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,3 +1,18 @@ +""" +Shared pytest fixtures for the ToolServer test suite. + +Provides a FastAPI TestClient wired with fixed, pre-authorized fake +delegated-execution identities (via require_workflow_execute / +require_runs_read dependency overrides) and a monkeypatched tool runner, +so most tests can exercise run/validate/results business logic without +real Auth/IAM connectivity or real tool network calls. Authentication +and authorization behavior itself is exercised separately, without this +override, in tests/test_toolserver_delegated_auth.py. + +Developer: + Manish Kumar +""" + from __future__ import annotations import time @@ -37,12 +52,14 @@ def _authorize(app) -> None: + """Override the delegated-execution dependencies with fixed, already-authorized fake identities so callers skip real Auth/IAM.""" app.dependency_overrides[require_workflow_execute] = lambda: FAKE_EXECUTE_IDENTITY app.dependency_overrides[require_runs_read] = lambda: FAKE_READ_IDENTITY @pytest.fixture() def client(monkeypatch, tmp_path): + """Build a TestClient with an isolated run-store directory, a network-free patched tool runner, and fake authorization already applied.""" # Ensure RunStore writes into temp monkeypatch.setenv("TOOLSERVER_RUN_STORE_DIR", str(tmp_path / "runs")) diff --git a/tests/test_app.py b/tests/test_app.py index 8533f41..0508977 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -8,6 +8,9 @@ Run with: python -m pytest tests/test_app.py -v + +Developer: + Manish Kumar """ from __future__ import annotations @@ -123,14 +126,18 @@ def ctx(monkeypatch, tmp_path): # =========================================================================== class TestHealth: + """GET /health reports service liveness with a stable response shape.""" def test_200(self, client): + """Health check responds with HTTP 200.""" assert client.get("/health").status_code == 200 def test_ok_true(self, client): + """Health check reports ok: true when the service is up.""" assert client.get("/health").json()["ok"] is True def test_service_key_present(self, client): + """Health response includes a service identifier field.""" assert "service" in client.get("/health").json() @@ -139,26 +146,33 @@ def test_service_key_present(self, client): # =========================================================================== class TestCapabilities: + """GET /capabilities advertises the engines and tool catalog callers can rely on.""" def test_200(self, client): + """Capabilities endpoint responds with HTTP 200.""" assert client.get("/capabilities").status_code == 200 def test_engines_present(self, client): + """Capabilities response includes an engines field.""" assert "engines" in client.get("/capabilities").json() def test_tools_present(self, client): + """Capabilities response includes a tools field.""" assert "tools" in client.get("/capabilities").json() def test_enrichr_pathway_listed(self, client): + """enrichr_pathway is advertised in the tool catalog.""" tool_ids = [t["tool_id"] for t in client.get("/capabilities").json()["tools"]] assert "enrichr_pathway" in tool_ids def test_enrichr_pathway_version_v1(self, client): + """enrichr_pathway is advertised at version v1.""" tools = client.get("/capabilities").json()["tools"] tool = next(t for t in tools if t["tool_id"] == "enrichr_pathway") assert tool["version"] == "v1" def test_enrichr_pathway_has_libraries_default_feature(self, client): + """enrichr_pathway advertises a libraries_default feature.""" tools = client.get("/capabilities").json()["tools"] tool = next(t for t in tools if t["tool_id"] == "enrichr_pathway") assert "libraries_default" in tool["features"] @@ -169,8 +183,10 @@ def test_enrichr_pathway_has_libraries_default_feature(self, client): # =========================================================================== class TestValidate: + """POST /validate checks a run request's shape without creating a run.""" def test_valid_inputs_ok_true(self, client): + """A well-formed request for a known tool validates as ok: true.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": VALID_INPUTS, @@ -180,10 +196,12 @@ def test_valid_inputs_ok_true(self, client): assert resp.json()["ok"] is True def test_unknown_tool_ok_false(self, client): + """Validating an unregistered tool_id reports ok: false.""" resp = client.post("/validate", json={"tool_id": "ghost", "inputs": {}, "resources": {}}) assert resp.json()["ok"] is False def test_unknown_tool_error_code(self, client): + """Validating an unregistered tool_id reports an UNKNOWN_TOOL error code.""" resp = client.post( "/validate", json={"tool_id": "ghost", "inputs": {}, "resources": {}} ) @@ -191,6 +209,7 @@ def test_unknown_tool_error_code(self, client): assert any(e["code"] == "UNKNOWN_TOOL" for e in errors) def test_empty_genes_list_ok_false(self, client): + """An empty genes list fails validation.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": {"genes": []}, @@ -199,6 +218,7 @@ def test_empty_genes_list_ok_false(self, client): assert resp.json()["ok"] is False def test_empty_genes_list_errors_non_empty(self, client): + """An empty genes list produces at least one validation error.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": {"genes": []}, @@ -207,6 +227,7 @@ def test_empty_genes_list_errors_non_empty(self, client): assert len(resp.json()["errors"]) > 0 def test_empty_genes_error_references_genes_field(self, client): + """The empty-genes validation error identifies the genes field.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": {"genes": []}, @@ -216,6 +237,7 @@ def test_empty_genes_error_references_genes_field(self, client): assert "genes" in fields def test_missing_genes_key_ok_false(self, client): + """Omitting the genes key entirely fails validation.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": {}, @@ -224,6 +246,7 @@ def test_missing_genes_key_ok_false(self, client): assert resp.json()["ok"] is False def test_genes_non_list_ok_false(self, client): + """A non-list genes value (e.g. a bare string) fails validation.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": {"genes": "TP53"}, @@ -232,6 +255,7 @@ def test_genes_non_list_ok_false(self, client): assert resp.json()["ok"] is False def test_warnings_key_always_present(self, client): + """The response always includes a warnings key, even when valid.""" resp = client.post("/validate", json={ "tool_id": "enrichr_pathway", "inputs": VALID_INPUTS, @@ -240,9 +264,11 @@ def test_warnings_key_always_present(self, client): assert "warnings" in resp.json() def test_missing_tool_id_returns_422(self, client): + """Omitting the required tool_id field is rejected at the HTTP layer with 422.""" assert client.post("/validate", json={"inputs": VALID_INPUTS}).status_code == 422 def test_omitting_inputs_does_not_422(self, client): + """Omitting inputs is legal because the model defaults it to an empty dict.""" # inputs has default_factory=dict, so omitting is legal at the HTTP layer resp = client.post("/validate", json={"tool_id": "enrichr_pathway"}) assert resp.status_code == 200 @@ -253,18 +279,23 @@ def test_omitting_inputs_does_not_422(self, client): # =========================================================================== class TestCreateRun: + """POST /runs validates the request, persists a run record, and executes it asynchronously.""" def test_returns_run_id(self, client): + """A successful submission returns a run_id.""" assert "run_id" in client.post("/runs", json=VALID_BODY).json() def test_run_id_has_ts_prefix(self, client): + """Generated run_id values are namespaced with a ts_ prefix.""" assert client.post("/runs", json=VALID_BODY).json()["run_id"].startswith("ts_") def test_run_ids_are_unique(self, client): + """Repeated submissions each get a distinct run_id.""" ids = {client.post("/runs", json=VALID_BODY).json()["run_id"] for _ in range(10)} assert len(ids) == 10 def test_empty_genes_returns_400(self, client): + """An empty genes list is rejected with HTTP 400 before a run is created.""" resp = client.post("/runs", json={ "tool_id": "enrichr_pathway", "inputs": {"genes": []}, @@ -273,6 +304,7 @@ def test_empty_genes_returns_400(self, client): assert resp.status_code == 400 def test_empty_genes_error_code_validation_failed(self, client): + """An empty genes list is rejected with a VALIDATION_FAILED error code.""" resp = client.post("/runs", json={ "tool_id": "enrichr_pathway", "inputs": {"genes": []}, @@ -281,51 +313,61 @@ def test_empty_genes_error_code_validation_failed(self, client): assert resp.json()["error"]["code"] == "VALIDATION_FAILED" def test_unknown_tool_returns_400(self, client): + """Submitting a run for an unregistered tool_id is rejected with HTTP 400.""" resp = client.post( "/runs", json={"tool_id": "ghost", "inputs": {}, "resources": {}} ) assert resp.status_code == 400 def test_missing_tool_id_returns_422(self, client): + """Omitting the required tool_id field is rejected at the HTTP layer with 422.""" assert client.post("/runs", json={"inputs": VALID_INPUTS}).status_code == 422 def test_record_created_in_store(self, ctx): + """A successful submission persists a run record in the run store.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert store.try_get(run_id) is not None def test_record_tool_id_is_enrichr_pathway(self, ctx): + """The persisted run record retains the submitted tool_id.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert store.get(run_id).tool_id == "enrichr_pathway" def test_record_has_queued_log(self, ctx): + """The persisted run record starts with a log line noting it was queued.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert any("Queued" in line for line in store.get(run_id).logs) def test_resources_stored_on_record(self, ctx): + """The submitted resources dict is persisted verbatim on the run record.""" client, store = ctx resources = {"cpu": 4} run_id = client.post("/runs", json={**VALID_BODY, "resources": resources}).json()["run_id"] assert store.get(run_id).resources == resources def test_inputs_stored_as_lightweight_placeholder(self, ctx): + """Full run inputs are not persisted on the record; only a placeholder summary is.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert store.get(run_id).inputs == {"summary": "stored externally"} def test_run_eventually_completes(self, ctx): + """A submitted run reaches the COMPLETED state once execution finishes.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "COMPLETED") def test_failing_run_reaches_failed_state(self, monkeypatch, tmp_path): + """A run whose tool execution raises reaches the FAILED state.""" client, store = _make_client(monkeypatch, tmp_path, run_fn=_failing_run) run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "FAILED") def test_status_200_on_success(self, client): + """A valid run submission responds with HTTP 200.""" assert client.post("/runs", json=VALID_BODY).status_code == 200 @@ -334,37 +376,46 @@ def test_status_200_on_success(self, client): # =========================================================================== class TestGetRun: + """GET /runs/{run_id} reports a run's current state, failing closed for unknown ids.""" def test_200_for_known_run(self, client): + """A known run_id responds with HTTP 200.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert client.get(f"/runs/{run_id}").status_code == 200 def test_run_id_echoed(self, client): + """The response echoes back the queried run_id.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert client.get(f"/runs/{run_id}").json()["run_id"] == run_id def test_state_is_valid_run_state(self, client): + """The reported state is always one of the documented run states.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] state = client.get(f"/runs/{run_id}").json()["state"] assert state in ("QUEUED", "RUNNING", "COMPLETED", "FAILED") def test_updated_epoch_present(self, client): + """The response includes an updated_epoch timestamp.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert "updated_epoch" in client.get(f"/runs/{run_id}").json() def test_unknown_run_id_state_is_failed(self, client): + """An unrecognized run_id is reported as FAILED rather than erroring.""" assert client.get("/runs/ghost-run-999").json()["state"] == "FAILED" def test_unknown_run_id_has_message(self, client): + """An unrecognized run_id's response includes an explanatory message.""" assert "message" in client.get("/runs/ghost-run-999").json() def test_state_completed_after_execution(self, ctx): + """Once execution finishes, the state endpoint reflects COMPLETED.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "COMPLETED") assert client.get(f"/runs/{run_id}").json()["state"] == "COMPLETED" def test_state_failed_after_execution_error(self, monkeypatch, tmp_path): + """Once execution raises, the state endpoint reflects FAILED.""" client, store = _make_client(monkeypatch, tmp_path, run_fn=_failing_run) run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "FAILED") @@ -376,23 +427,29 @@ def test_state_failed_after_execution_error(self, monkeypatch, tmp_path): # =========================================================================== class TestGetLogs: + """GET /runs/{run_id}/logs returns a tailable, newline-joined log stream.""" def test_200_for_known_run(self, client): + """A known run_id responds with HTTP 200.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert client.get(f"/runs/{run_id}/logs").status_code == 200 def test_run_id_echoed(self, client): + """The response echoes back the queried run_id.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert client.get(f"/runs/{run_id}/logs").json()["run_id"] == run_id def test_logs_field_is_string(self, client): + """The logs field is returned as a single joined string, not a list.""" run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] assert isinstance(client.get(f"/runs/{run_id}/logs").json()["logs"], str) def test_unknown_run_contains_unknown_run_message(self, client): + """An unrecognized run_id's logs field explains the run is unknown.""" assert "unknown run" in client.get("/runs/ghost-999/logs").json()["logs"] def test_fake_run_log_line_appears_after_completion(self, ctx): + """Log lines emitted by the tool's execution are present once it completes.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "COMPLETED") @@ -400,6 +457,7 @@ def test_fake_run_log_line_appears_after_completion(self, ctx): assert "FAKE RUN called" in logs def test_tail_limits_line_count(self, ctx): + """The tail query parameter caps the number of log lines returned.""" client, store = ctx rec = RunRecord( run_id="log-many", tool_id="enrichr_pathway", state="COMPLETED", @@ -413,6 +471,7 @@ def test_tail_limits_line_count(self, ctx): assert len(data["logs"].strip().splitlines()) == 5 def test_tail_returns_last_lines(self, ctx): + """Tailing returns the most recent lines, not the earliest ones.""" client, store = ctx rec = RunRecord( run_id="log-tail", tool_id="enrichr_pathway", state="COMPLETED", @@ -427,6 +486,7 @@ def test_tail_returns_last_lines(self, ctx): assert "line-0" not in data["logs"] def test_default_tail_is_200_lines(self, ctx): + """With no tail parameter, the log stream defaults to the last 200 lines.""" client, store = ctx rec = RunRecord( run_id="log-default", tool_id="enrichr_pathway", state="COMPLETED", @@ -459,9 +519,12 @@ def test_tail_zero_returns_all_lines(self, ctx): # =========================================================================== class TestGetResults: + """GET /runs/{run_id}/results gates access to results by run state.""" def _inject(self, store: RunStore, run_id: str, state: str, results=None, error=None) -> None: + """Seed the store directly with a run record in an arbitrary state, + bypassing the /runs endpoint so state-gating can be tested in isolation.""" store.create(RunRecord( run_id=run_id, tool_id="enrichr_pathway", state=state, created_epoch=1_700_000_000, updated_epoch=1_700_000_001, @@ -470,11 +533,13 @@ def _inject(self, store: RunStore, run_id: str, state: str, )) def test_unknown_run_not_found_code(self, client): + """An unrecognized run_id reports ok: false with a NOT_FOUND error code.""" data = client.get("/runs/ghost-999/results").json() assert data["ok"] is False assert data["error"]["code"] == "NOT_FOUND" def test_queued_run_not_ready_code(self, ctx): + """A QUEUED run reports ok: false with a NOT_READY error code.""" client, store = ctx self._inject(store, "q-run", "QUEUED") data = client.get("/runs/q-run/results").json() @@ -482,6 +547,7 @@ def test_queued_run_not_ready_code(self, ctx): assert data["error"]["code"] == "NOT_READY" def test_running_run_not_ready_code(self, ctx): + """A RUNNING run reports ok: false with a NOT_READY error code.""" client, store = ctx self._inject(store, "r-run", "RUNNING") data = client.get("/runs/r-run/results").json() @@ -489,34 +555,40 @@ def test_running_run_not_ready_code(self, ctx): assert data["error"]["code"] == "NOT_READY" def test_not_ready_echoes_state(self, ctx): + """A not-ready response still echoes the run's actual current state.""" client, store = ctx self._inject(store, "s-run", "RUNNING") assert client.get("/runs/s-run/results").json()["state"] == "RUNNING" def test_failed_run_not_ready(self, ctx): + """A FAILED run's results are reported as not ok rather than returning stale data.""" client, store = ctx self._inject(store, "f-run", "FAILED", error={"code": "EXEC_FAILED", "message": "oops", "trace": ""}) assert client.get("/runs/f-run/results").json()["ok"] is False def test_completed_returns_stored_results(self, ctx): + """A COMPLETED run returns exactly the results stored on its record.""" client, store = ctx expected = {"score": 99, "label": "hit"} self._inject(store, "done-run", "COMPLETED", results=expected) assert client.get("/runs/done-run/results").json() == expected def test_completed_none_results_returns_default_ok(self, ctx): + """A COMPLETED run with no stored results still returns a default ok: true body.""" client, store = ctx self._inject(store, "done-empty", "COMPLETED", results=None) assert client.get("/runs/done-empty/results").json()["ok"] is True def test_e2e_results_ok_true(self, ctx): + """End-to-end: a submitted run's results are ok: true once execution completes.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "COMPLETED") assert client.get(f"/runs/{run_id}/results").json()["ok"] is True def test_e2e_results_contain_wikipathways_key(self, ctx): + """End-to-end: the completed run's results contain the tool's expected result key.""" client, store = ctx run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "COMPLETED") @@ -524,6 +596,7 @@ def test_e2e_results_contain_wikipathways_key(self, ctx): assert "WikiPathways_2024_Human" in data.get("results", {}) def test_e2e_failed_run_results_not_ready(self, monkeypatch, tmp_path): + """End-to-end: a run whose execution raises never exposes results as ok.""" client, store = _make_client(monkeypatch, tmp_path, run_fn=_failing_run) run_id = client.post("/runs", json=VALID_BODY).json()["run_id"] _wait_for_state(store, run_id, "FAILED") @@ -546,6 +619,7 @@ def test_e2e_failed_run_results_not_ready(self, monkeypatch, tmp_path): # test_toolserver_app_coverage.py's register_tools tests. class TestRegisterToolsDisabled: + """POST /register_tools is unconditionally disabled, with no delegated permission able to unlock it.""" def test_register_tools_returns_501_even_when_authenticated(self, ctx): """An authenticated caller (this file's fixtures always are, via @@ -557,6 +631,9 @@ def test_register_tools_returns_501_even_when_authenticated(self, ctx): assert resp.status_code == 501 def test_register_tools_disabled_leaves_tool_unregistered(self, ctx): + """Since /register_tools is a no-op, a "registered" tool_id never + actually enters the registry -- a run submitted against it fails + closed with UNKNOWN_TOOL, not a stub-execution NotImplementedError.""" client, _ = ctx client.post("/register_tools", json={"tools": [{"tool_id": "stub_exec_tool"}]}) resp = client.post("/runs", json={ diff --git a/tests/test_david_annotation.py b/tests/test_david_annotation.py index e6b7397..531fded 100644 --- a/tests/test_david_annotation.py +++ b/tests/test_david_annotation.py @@ -1,3 +1,16 @@ +"""Tests for toolserver/tools/david_annotation.py, the DAVID functional +annotation tool adapter. + +Covers input validation (_validate), the raw SOAP request/response +mechanics against the DAVID web service (_soap), chart-record XML parsing +with and without XML namespaces (_parse_chart_records), and the end-to-end +tool run that authenticates, submits a gene list, and retrieves formatted +enrichment results (_run). + +Developer: + Manish Kumar +""" + from __future__ import annotations from unittest.mock import MagicMock, patch @@ -13,36 +26,45 @@ # ═════════════════════════════════════════════════════════════════════════════ class TestValidate: + """_validate's required-field checks for the DAVID annotation tool's inputs.""" + def test_valid_inputs_pass(self): + """A request with both email and gene_ids passes with no errors or warnings.""" result = _validate({"email": "user@example.com", "gene_ids": "1,2,3"}, {}) assert result["ok"] is True assert result["errors"] == [] assert result["warnings"] == [] def test_missing_email_fails(self): + """Omitting email fails validation and reports an error on the email field.""" result = _validate({"gene_ids": "1,2,3"}, {}) assert result["ok"] is False assert any(e["field"] == "email" for e in result["errors"]) def test_empty_email_fails(self): + """An empty-string email fails validation.""" result = _validate({"email": "", "gene_ids": "1,2,3"}, {}) assert result["ok"] is False def test_missing_gene_ids_fails(self): + """Omitting gene_ids fails validation and reports an error on the gene_ids field.""" result = _validate({"email": "user@example.com"}, {}) assert result["ok"] is False assert any(e["field"] == "gene_ids" for e in result["errors"]) def test_empty_gene_ids_fails(self): + """An empty gene_ids list fails validation.""" result = _validate({"email": "user@example.com", "gene_ids": []}, {}) assert result["ok"] is False def test_missing_both_gives_two_errors(self): + """Omitting both required fields reports exactly one error per field.""" result = _validate({}, {}) assert result["ok"] is False assert len(result["errors"]) == 2 def test_error_message_references_field(self): + """Each validation error names the specific field it applies to.""" result = _validate({}, {}) fields = {e["field"] for e in result["errors"]} assert fields == {"email", "gene_ids"} @@ -53,6 +75,8 @@ def test_error_message_references_field(self): # ═════════════════════════════════════════════════════════════════════════════ class TestSoap: + """_soap's construction of DAVID SOAP requests and handling of responses.""" + def _make_client(self, response_text="", raise_error=False): mock_client = MagicMock() mock_resp = MagicMock() @@ -65,40 +89,47 @@ def _make_client(self, response_text="", raise_error=False): return mock_client def test_posts_to_david_ws_url(self): + """The request is posted to the DAVID web service URL.""" client = self._make_client() _soap(client, "authenticate", "") url = client.post.call_args[0][0] assert "DAVIDWebService" in url def test_sets_content_type_header(self): + """The request declares a text/xml Content-Type header.""" client = self._make_client() _soap(client, "action", "") headers = client.post.call_args[1]["headers"] assert headers["Content-Type"] == "text/xml" def test_sets_soap_action_header(self): + """The SOAPAction header carries the requested action name.""" client = self._make_client() _soap(client, "myAction", "") headers = client.post.call_args[1]["headers"] assert "myAction" in headers["SOAPAction"] def test_includes_body_in_envelope(self): + """The caller-supplied SOAP body is embedded in the request envelope.""" client = self._make_client() _soap(client, "action", "data") content = client.post.call_args[1]["content"] assert "data" in content def test_returns_response_text(self): + """_soap returns the raw response text unchanged.""" client = self._make_client("expected_response_text") result = _soap(client, "action", "") assert result == "expected_response_text" def test_raise_for_status_called(self): + """_soap checks the HTTP response status before returning.""" client = self._make_client() _soap(client, "action", "") client.post.return_value.raise_for_status.assert_called_once() def test_propagates_http_error(self): + """An HTTP error status from DAVID propagates out of _soap unhandled.""" client = self._make_client(raise_error=True) with pytest.raises(httpx.HTTPStatusError): _soap(client, "action", "") @@ -109,13 +140,17 @@ def test_propagates_http_error(self): # ═════════════════════════════════════════════════════════════════════════════ class TestParseChartRecords: + """_parse_chart_records's extraction of records from DAVID chart XML.""" + def test_bare_return_element(self): + """A element with no XML namespace is parsed into a record.""" xml = "GO:0001" records = _parse_chart_records(xml) assert len(records) == 1 assert records[0]["termName"] == "GO:0001" def test_namespaced_return_element(self): + """A namespaced element is parsed the same as a bare one.""" xml = ( '' "GO:0002" @@ -126,6 +161,7 @@ def test_namespaced_return_element(self): assert records[0]["termName"] == "GO:0002" def test_mixed_namespaced_and_bare_children(self): + """A element's namespaced and bare child tags are both captured.""" xml = ( '' "BPGO:003" @@ -136,6 +172,7 @@ def test_mixed_namespaced_and_bare_children(self): assert records[0]["term"] == "GO:003" def test_multiple_records(self): + """Multiple elements each produce their own record, in document order.""" xml = ( "" "A" @@ -148,20 +185,24 @@ def test_multiple_records(self): assert records[1]["t"] == "B" def test_invalid_xml_returns_empty(self): + """Malformed XML yields an empty record list rather than raising.""" records = _parse_chart_records("<<>>") assert records == [] def test_empty_string_returns_empty(self): + """An empty input string yields an empty record list rather than raising.""" records = _parse_chart_records("") assert records == [] def test_empty_return_element_skipped(self): + """A element with no child fields is skipped, not returned as an empty record.""" xml = "valid" records = _parse_chart_records(xml) assert len(records) == 1 assert records[0]["t"] == "valid" def test_no_return_elements_returns_empty(self): + """XML with no elements at all yields an empty record list.""" xml = ( "" @@ -172,6 +213,7 @@ def test_no_return_elements_returns_empty(self): assert records == [] def test_all_fields_extracted(self): + """Every child field of a element is extracted into the record.""" xml = ( "" "GOTERM_BP" @@ -195,11 +237,14 @@ def test_all_fields_extracted(self): # ═════════════════════════════════════════════════════════════════════════════ class TestRun: + """_run's end-to-end DAVID annotation flow: authenticate, submit, fetch chart, format.""" + def _soap_iter(self, responses): it = iter(responses) return lambda *args, **kwargs: next(it) def test_successful_run_returns_ok(self): + """A full successful run (auth, addList, setCategories, getChartReport) reports ok.""" responses = ["true", "", "", ""] with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): result = _run({"email": "test@lab.com", "gene_ids": "1,2,3"}, {}, MagicMock()) @@ -208,6 +253,7 @@ def test_successful_run_returns_ok(self): assert result["results"] == [] def test_run_meta_reflects_inputs(self): + """The run's meta block reflects the caller-supplied email and id_type.""" responses = ["true", "", "", ""] with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): result = _run( @@ -218,6 +264,7 @@ def test_run_meta_reflects_inputs(self): assert result["meta"]["id_type"] == "GENE_SYMBOL" def test_custom_params_reflected_in_meta(self): + """Custom threshold, count, and categories parameters are reflected in the run's meta block.""" responses = ["true", "", "", ""] with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): result = _run( @@ -238,18 +285,21 @@ def test_custom_params_reflected_in_meta(self): assert result["meta"]["categories"] == "KEGG_PATHWAY" def test_auth_failure_raises_runtime_error(self): + """A DAVID authentication failure raises RuntimeError instead of proceeding.""" responses = ["FALSE"] with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): with pytest.raises(RuntimeError, match="authentication failed"): _run({"email": "bad@test.com", "gene_ids": "1"}, {}, MagicMock()) def test_addlist_fault_raises_runtime_error(self): + """A SOAP Fault from the addList step raises RuntimeError instead of proceeding.""" responses = ["true", "bad gene list"] with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): with pytest.raises(RuntimeError, match="addList failed"): _run({"email": "test@test.com", "gene_ids": "bad"}, {}, MagicMock()) def test_chart_records_are_parsed_and_formatted(self): + """A single chart record from DAVID is parsed and reformatted into the run's result fields.""" chart_xml = ( "" "GOTERM_BP_DIRECT" @@ -279,6 +329,7 @@ def test_chart_records_are_parsed_and_formatted(self): assert r["bonferroni"] == "0.003" def test_multiple_chart_records(self): + """Multiple chart records from DAVID are each parsed into their own result entry, in order.""" chart_xml = ( "" "GO_BPT1" @@ -293,6 +344,7 @@ def test_multiple_chart_records(self): assert result["results"][1]["category"] == "KEGG" def test_log_called_during_run(self): + """The caller-supplied log callback is invoked at each step of the run.""" responses = ["true", "", "", ""] log = MagicMock() with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): @@ -300,6 +352,7 @@ def test_log_called_during_run(self): assert log.call_count >= 4 def test_default_parameters_applied(self): + """Omitted optional parameters fall back to DAVID's documented defaults.""" responses = ["true", "", "", ""] with patch("toolserver.tools.david_annotation._soap", side_effect=self._soap_iter(responses)): result = _run({"email": "test@test.com", "gene_ids": "1,2"}, {}, MagicMock()) diff --git a/tests/test_endpoints.py b/tests/test_endpoints.py index 4514ac5..cc3d6c5 100644 --- a/tests/test_endpoints.py +++ b/tests/test_endpoints.py @@ -1,4 +1,14 @@ +""" +Tests for ToolServer's core HTTP endpoints: health, capabilities, +validation, and the create-run-to-completion happy path. + +Developer: + Manish Kumar +""" + + def test_health(client): + """Return 200 with ok=True and a service identifier.""" r = client.get("/health") assert r.status_code == 200 body = r.json() @@ -7,6 +17,7 @@ def test_health(client): def test_capabilities(client): + """List registered engines and tools, including enrichr_pathway.""" r = client.get("/capabilities") assert r.status_code == 200 caps = r.json() @@ -17,6 +28,7 @@ def test_capabilities(client): def test_validate_unknown_tool(client): + """Report UNKNOWN_TOOL as a validation error, not an HTTP failure, for an unregistered tool_id.""" r = client.post("/validate", json={"tool_id": "nope", "inputs": {}, "resources": {}}) assert r.status_code == 200 body = r.json() @@ -25,6 +37,7 @@ def test_validate_unknown_tool(client): def test_validate_enrichr_pathway_requires_genes(client): + """Flag the missing required "genes" field when validating enrichr_pathway inputs.""" r = client.post("/validate", json={"tool_id": "enrichr_pathway", "inputs": {}, "resources": {}}) assert r.status_code == 200 body = r.json() @@ -33,6 +46,7 @@ def test_validate_enrichr_pathway_requires_genes(client): def test_create_run_validation_fails(client): + """Reject run creation with 400 VALIDATION_FAILED when required inputs are missing.""" r = client.post("/runs", json={"tool_id": "enrichr_pathway", "inputs": {}, "resources": {}}) assert r.status_code == 400 body = r.json() @@ -41,6 +55,7 @@ def test_create_run_validation_fails(client): def test_create_run_and_poll_to_complete(client): + """Drive a valid run from creation through COMPLETED state, with logs and results populated along the way.""" req = { "tool_id": "enrichr_pathway", "inputs": {"genes": ["TP53", "BRCA1"], "top_n": 10}, diff --git a/tests/test_enrichr_tool.py b/tests/test_enrichr_tool.py index 0537ed2..c0cee35 100644 --- a/tests/test_enrichr_tool.py +++ b/tests/test_enrichr_tool.py @@ -1,9 +1,22 @@ """ -Unit tests for the Enrichr gene-set enrichment tool. +Unit tests for the Enrichr gene-set enrichment tool +(toolserver/tools/enrichr_pathway.py). + +Covers request validation (_validate: gene list, library list, top_n bounds, +sort_by whitelist), Enrichr response parsing (_row_to_item, +_normalize_enrichr_payload: malformed/short/non-numeric rows fail closed to +None/skipped rather than raising), result ordering and truncation +(_sort_and_top), and the end-to-end _run path against a mocked httpx.Client +(addList/enrich HTTP-error propagation, gene-list sanitization, top_n and +return_mode handling, default libraries, and the response shape returned to +callers). Run with: pip install pytest httpx pytest-cov python -m pytest tests/test_enrichr_tool.py -v + +Developer: + Manish Kumar """ from __future__ import annotations @@ -72,115 +85,143 @@ def _make_items(n: int) -> List[Dict[str, Any]]: # =========================================================================== class TestValidate: + """_validate's request-validation contract: genes/libraries/top_n/sort_by + are checked independently, every failure is reported (not just the + first), and a warnings list is always returned.""" # --- genes --- def test_valid_minimal(self): + """A minimal request with only a non-empty gene list validates ok with no errors.""" result = _validate({"genes": ["TP53", "BRCA1"]}, {}) assert result["ok"] is True assert result["errors"] == [] def test_genes_missing(self): + """A request with no "genes" key fails validation with a "genes" field error.""" result = _validate({}, {}) assert result["ok"] is False fields = [e["field"] for e in result["errors"]] assert "genes" in fields def test_genes_empty_list(self): + """An empty gene list fails validation.""" result = _validate({"genes": []}, {}) assert result["ok"] is False def test_genes_not_a_list(self): + """A non-list "genes" value (e.g. a bare string) fails validation.""" result = _validate({"genes": "TP53"}, {}) assert result["ok"] is False def test_genes_list_with_blank_string(self): + """A gene list containing a whitespace-only entry fails validation.""" result = _validate({"genes": ["TP53", " "]}, {}) assert result["ok"] is False def test_genes_non_string_elements(self): + """A gene list containing a non-string element fails validation.""" result = _validate({"genes": [123, "BRCA1"]}, {}) assert result["ok"] is False # --- libraries --- def test_libraries_none_allowed(self): + """Omitting libraries (None) is valid -- the tool falls back to defaults.""" result = _validate({"genes": ["TP53"], "libraries": None}, {}) assert result["ok"] is True def test_libraries_valid(self): + """A non-empty list of library names validates ok.""" result = _validate({"genes": ["TP53"], "libraries": ["KEGG_2021_Human"]}, {}) assert result["ok"] is True def test_libraries_empty_list(self): + """An explicit empty libraries list fails validation (use None to accept defaults, not []).""" result = _validate({"genes": ["TP53"], "libraries": []}, {}) assert result["ok"] is False def test_libraries_not_a_list(self): + """A non-list libraries value fails validation.""" result = _validate({"genes": ["TP53"], "libraries": "KEGG_2021_Human"}, {}) assert result["ok"] is False def test_libraries_blank_element(self): + """A libraries list containing a whitespace-only entry fails validation.""" result = _validate({"genes": ["TP53"], "libraries": [" "]}, {}) assert result["ok"] is False # --- top_n --- def test_top_n_valid(self): + """An in-range integer top_n validates ok.""" result = _validate({"genes": ["TP53"], "top_n": 10}, {}) assert result["ok"] is True def test_top_n_as_string_integer(self): + """A numeric string top_n (e.g. "50") is coerced and validates ok.""" result = _validate({"genes": ["TP53"], "top_n": "50"}, {}) assert result["ok"] is True def test_top_n_zero(self): + """top_n of 0 is below the minimum of 1 and fails validation.""" result = _validate({"genes": ["TP53"], "top_n": 0}, {}) assert result["ok"] is False def test_top_n_above_500(self): + """top_n above the 500 ceiling fails validation.""" result = _validate({"genes": ["TP53"], "top_n": 501}, {}) assert result["ok"] is False def test_top_n_boundary_1(self): + """top_n of exactly 1 (the minimum) validates ok.""" result = _validate({"genes": ["TP53"], "top_n": 1}, {}) assert result["ok"] is True def test_top_n_boundary_500(self): + """top_n of exactly 500 (the maximum) validates ok.""" result = _validate({"genes": ["TP53"], "top_n": 500}, {}) assert result["ok"] is True def test_top_n_non_numeric(self): + """A non-numeric top_n string fails validation.""" result = _validate({"genes": ["TP53"], "top_n": "abc"}, {}) assert result["ok"] is False # --- sort_by --- def test_sort_by_adj_p_value(self): + """sort_by="adj_p_value" is an allowed sort key.""" result = _validate({"genes": ["TP53"], "sort_by": "adj_p_value"}, {}) assert result["ok"] is True def test_sort_by_p_value(self): + """sort_by="p_value" is an allowed sort key.""" result = _validate({"genes": ["TP53"], "sort_by": "p_value"}, {}) assert result["ok"] is True def test_sort_by_combined_score(self): + """sort_by="combined_score" is an allowed sort key.""" result = _validate({"genes": ["TP53"], "sort_by": "combined_score"}, {}) assert result["ok"] is True def test_sort_by_invalid(self): + """A sort_by value outside the allowed set fails validation.""" result = _validate({"genes": ["TP53"], "sort_by": "fdr"}, {}) assert result["ok"] is False # --- warnings list always present --- def test_warnings_always_present(self): + """The result always includes a "warnings" key, even when there are no errors.""" result = _validate({"genes": ["TP53"]}, {}) assert "warnings" in result # --- multiple errors --- def test_multiple_errors_accumulated(self): + """Independent field errors (genes, top_n, sort_by) are all reported together, + not short-circuited on the first failure.""" result = _validate({"genes": [], "top_n": -1, "sort_by": "bad"}, {}) fields = [e["field"] for e in result["errors"]] assert "genes" in fields @@ -193,8 +234,13 @@ def test_multiple_errors_accumulated(self): # =========================================================================== class TestRowToItem: + """_row_to_item's per-row parsing contract: a well-formed 9- or 7-element + Enrichr row becomes a structured item; malformed fields (bad term, bad + numeric fields, bad overlap-genes shape) fail closed to None (the whole + row, or just that field) rather than raising.""" def test_full_row_parsed(self): + """A full 9-element row is parsed into an item with all fields populated and typed.""" row = _make_row() item = _row_to_item(row) assert item is not None @@ -209,6 +255,8 @@ def test_full_row_parsed(self): assert item["old_adj_p_value"] == pytest.approx(0.05) def test_short_row_7_elements(self): + """A 7-element row (missing the trailing old_p_value/old_adj_p_value + columns) still parses, with those two fields set to None.""" row = _make_row()[:7] item = _row_to_item(row) assert item is not None @@ -216,21 +264,27 @@ def test_short_row_7_elements(self): assert item["old_adj_p_value"] is None def test_row_too_short_returns_none(self): + """A row with fewer than 7 elements is rejected outright (returns None).""" assert _row_to_item([1, "Term", 0.01]) is None def test_non_list_returns_none(self): + """A non-list row value (string or None) returns None instead of raising.""" assert _row_to_item("not a list") is None # type: ignore assert _row_to_item(None) is None # type: ignore def test_blank_term_returns_none(self): + """A row whose term is whitespace-only is rejected (returns None).""" row = _make_row(term=" ") assert _row_to_item(row) is None def test_empty_term_returns_none(self): + """A row whose term is an empty string is rejected (returns None).""" row = _make_row(term="") assert _row_to_item(row) is None def test_non_numeric_p_value_becomes_none(self): + """A non-numeric p_value field becomes None on the item rather than + rejecting the whole row.""" row = _make_row() row[2] = "not_a_float" item = _row_to_item(row) @@ -238,6 +292,8 @@ def test_non_numeric_p_value_becomes_none(self): assert item["p_value"] is None def test_non_numeric_rank_becomes_none(self): + """A non-numeric rank field becomes None on the item rather than + rejecting the whole row.""" row = _make_row() row[0] = "one" item = _row_to_item(row) @@ -245,6 +301,8 @@ def test_non_numeric_rank_becomes_none(self): assert item["rank"] is None def test_overlap_genes_non_list_becomes_empty(self): + """A non-list overlap_genes field (e.g. a delimited string) becomes an + empty list on the item rather than being passed through as-is.""" row = _make_row() row[5] = "GENE1;GENE2" # string instead of list item = _row_to_item(row) @@ -252,6 +310,7 @@ def test_overlap_genes_non_list_becomes_empty(self): assert item["overlap_genes"] == [] def test_overlap_genes_list_preserved(self): + """A well-formed overlap_genes list is preserved unchanged on the item.""" row = _make_row(overlap_genes=["A", "B", "C"]) item = _row_to_item(row) assert item["overlap_genes"] == ["A", "B", "C"] @@ -262,8 +321,13 @@ def test_overlap_genes_list_preserved(self): # =========================================================================== class TestNormalizeEnrichrPayload: + """_normalize_enrichr_payload's contract: turn a raw Enrichr API payload + for one library into a {library, columns, n_terms, items} structure, + dropping rows that _row_to_item rejects and tolerating a missing library + key or a None payload instead of raising.""" def test_basic_normalization(self): + """Two valid rows for a library normalize into 2 items with matching n_terms.""" lib = "WikiPathways_2024_Human" payload = {lib: [_make_row(), _make_row(rank=2, term="Pathway B", adj_p_value=0.1)]} result = _normalize_enrichr_payload(payload, lib) @@ -273,22 +337,26 @@ def test_basic_normalization(self): assert "columns" in result def test_empty_library_key(self): + """A library key present with an empty row list normalizes to zero items.""" lib = "WikiPathways_2024_Human" result = _normalize_enrichr_payload({lib: []}, lib) assert result["n_terms"] == 0 assert result["items"] == [] def test_missing_library_key(self): + """A payload missing the requested library key normalizes to zero items, not a KeyError.""" result = _normalize_enrichr_payload({}, "SomeLib") assert result["n_terms"] == 0 def test_invalid_rows_skipped(self): + """A row that _row_to_item rejects (here: blank term) is dropped, leaving only valid rows.""" lib = "WikiPathways_2024_Human" payload = {lib: [[1, "", 0.01, -1, 10, [], 0.05], _make_row()]} result = _normalize_enrichr_payload(payload, lib) assert result["n_terms"] == 1 def test_columns_list(self): + """The returned columns list names all 9 item fields in a fixed, stable order.""" lib = "Reactome_2022" result = _normalize_enrichr_payload({lib: [_make_row()]}, lib) expected_cols = [ @@ -298,6 +366,7 @@ def test_columns_list(self): assert result["columns"] == expected_cols def test_none_payload_handled(self): + """A None payload normalizes to zero items instead of raising.""" result = _normalize_enrichr_payload(None, "SomeLib") # type: ignore assert result["n_terms"] == 0 @@ -307,45 +376,57 @@ def test_none_payload_handled(self): # =========================================================================== class TestSortAndTop: + """_sort_and_top's ordering/truncation contract: adj_p_value and p_value + sort ascending, combined_score sorts descending, items with a None sort + key sort last, an unrecognized sort_by falls back to adj_p_value + ordering, and top_n truncates the result to at least 1 item.""" def test_sort_by_adj_p_value_ascending(self): + """sort_by="adj_p_value" orders items by ascending adj_p_value.""" items = _make_items(5) result = _sort_and_top(items, sort_by="adj_p_value", top_n=5) vals = [r["adj_p_value"] for r in result] assert vals == sorted(vals) def test_sort_by_p_value_ascending(self): + """sort_by="p_value" orders items by ascending p_value.""" items = _make_items(5) result = _sort_and_top(items, sort_by="p_value", top_n=5) vals = [r["p_value"] for r in result] assert vals == sorted(vals) def test_sort_by_combined_score_descending(self): + """sort_by="combined_score" orders items by descending combined_score.""" items = _make_items(5) result = _sort_and_top(items, sort_by="combined_score", top_n=5) vals = [r["combined_score"] for r in result] assert vals == sorted(vals, reverse=True) def test_top_n_limits_output(self): + """top_n truncates a larger result set down to exactly top_n items.""" items = _make_items(10) result = _sort_and_top(items, sort_by="adj_p_value", top_n=3) assert len(result) == 3 def test_top_n_larger_than_items(self): + """A top_n larger than the item count returns all items, not padded or duplicated.""" items = _make_items(3) result = _sort_and_top(items, sort_by="adj_p_value", top_n=10) assert len(result) == 3 def test_top_n_minimum_1(self): + """A top_n of 0 still returns at least 1 item, not an empty result.""" items = _make_items(5) result = _sort_and_top(items, sort_by="adj_p_value", top_n=0) assert len(result) >= 1 def test_empty_items(self): + """Sorting an empty item list returns an empty list.""" result = _sort_and_top([], sort_by="adj_p_value", top_n=10) assert result == [] def test_none_adj_p_value_sorted_last(self): + """An item with adj_p_value=None sorts after items with real values, not first.""" items = [ {**_make_items(1)[0], "adj_p_value": None, "p_value": None, "combined_score": None, "term": "Null Item"}, @@ -355,6 +436,8 @@ def test_none_adj_p_value_sorted_last(self): assert result[0]["term"] == "Good Item" def test_default_sort_key_is_adj_p_value(self): + """An unrecognized sort_by value falls back to the same ordering as + sort_by="adj_p_value" rather than raising or leaving items unsorted.""" # Anything other than combined_score / p_value falls back to adj_p_value ordering items = _make_items(4) r1 = _sort_and_top(items, sort_by="adj_p_value", top_n=4) @@ -403,6 +486,8 @@ def _setup_mock_client(self, mock_client_cls, lib: str): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_successful_run_returns_ok(self, mock_client_cls): + """A successful addList + enrich round trip returns ok=True with the + userListId and per-library results populated.""" self._setup_mock_client(mock_client_cls, self.LIB) result = _run( {"genes": ["TP53", "BRCA1"], "libraries": [self.LIB]}, @@ -415,6 +500,7 @@ def test_successful_run_returns_ok(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_meta_populated(self, mock_client_cls): + """result["meta"] echoes back the effective n_genes, top_n, sort_by, and libraries used.""" self._setup_mock_client(mock_client_cls, self.LIB) result = _run( {"genes": ["TP53", "BRCA1"], "libraries": [self.LIB], "top_n": 3, "sort_by": "p_value"}, @@ -428,6 +514,7 @@ def test_meta_populated(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_top_n_respected(self, mock_client_cls): + """The end-to-end _run path truncates each library's items to top_n.""" self._setup_mock_client(mock_client_cls, self.LIB) result = _run( {"genes": ["TP53", "BRCA1"], "libraries": [self.LIB], "top_n": 2}, @@ -439,6 +526,7 @@ def test_top_n_respected(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_return_mode_all_does_not_apply_top_n(self, mock_client_cls): + """return_mode="all" bypasses top_n truncation and returns every parsed row.""" self._setup_mock_client(mock_client_cls, self.LIB) result = _run( { @@ -456,6 +544,8 @@ def test_return_mode_all_does_not_apply_top_n(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_genes_stripped_and_blanks_removed(self, mock_client_cls): + """Gene names are whitespace-trimmed and blank entries are dropped + before being posted to Enrichr's addList endpoint.""" mock_client = self._setup_mock_client(mock_client_cls, self.LIB) _run( {"genes": [" TP53 ", "", "BRCA1"], "libraries": [self.LIB]}, @@ -474,6 +564,8 @@ def test_genes_stripped_and_blanks_removed(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_default_libraries_used_when_not_provided(self, mock_client_cls): + """Omitting "libraries" from the request falls back to querying the + tool's built-in default library set (WikiPathways and Reactome).""" # Need to handle two library calls; return the first lib's response for any get mock_client = MagicMock() mock_client_cls.return_value.__enter__.return_value = mock_client @@ -498,6 +590,8 @@ def enrich_side_effect(url, params=None): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_addlist_http_error_raises(self, mock_client_cls): + """A non-2xx response from Enrichr's addList endpoint raises RuntimeError + instead of continuing with a broken userListId.""" mock_client = MagicMock() mock_client_cls.return_value.__enter__.return_value = mock_client @@ -511,6 +605,7 @@ def test_addlist_http_error_raises(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_missing_user_list_id_raises(self, mock_client_cls): + """An addList response missing the expected userListId key raises RuntimeError.""" mock_client = MagicMock() mock_client_cls.return_value.__enter__.return_value = mock_client @@ -524,6 +619,8 @@ def test_missing_user_list_id_raises(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_enrich_http_error_raises(self, mock_client_cls): + """A non-2xx response from Enrichr's enrich endpoint raises RuntimeError + instead of returning a partial or empty result.""" mock_client = MagicMock() mock_client_cls.return_value.__enter__.return_value = mock_client @@ -542,6 +639,8 @@ def test_enrich_http_error_raises(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_log_callable_called(self, mock_client_cls): + """The caller-supplied log callback is invoked at least once per HTTP + stage (addList and enrich) during a successful run.""" self._setup_mock_client(mock_client_cls, self.LIB) log_messages = [] _run( @@ -553,6 +652,8 @@ def test_log_callable_called(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_custom_base_url_used(self, mock_client_cls): + """A "_enrichr_base_url" override in the request is used for the addList + HTTP call instead of the tool's default Enrichr host.""" mock_client = self._setup_mock_client(mock_client_cls, self.LIB) _run( { @@ -568,6 +669,8 @@ def test_custom_base_url_used(self, mock_client_cls): @patch("toolserver.tools.enrichr_pathway.httpx.Client") def test_result_structure(self, mock_client_cls): + """Each per-library result carries the full expected key set + (library, columns, sort_by, top_n, n_terms, items).""" self._setup_mock_client(mock_client_cls, self.LIB) result = _run( {"genes": ["TP53", "MYC"], "libraries": [self.LIB]}, diff --git a/tests/test_executor.py b/tests/test_executor.py index cfb79fb..50edb9d 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -1,8 +1,19 @@ """ Unit tests for toolserver.executor.Executor. +Covers the thread-pool-backed execution lifecycle: dependency wiring at +construction, the happy-path state transition from QUEUED through RUNNING +to COMPLETED, the failure path that captures handler exceptions into a +structured FAILED error record, registry-based handler lookup by tool_id +(an execution-boundary contract -- an unregistered tool must never run), +log-line persistence via the handler's log callback, and concurrent +execution of independent jobs. + Run with: python -m pytest tests/test_executor.py -v + +Developer: + Manish Kumar """ from __future__ import annotations @@ -115,21 +126,27 @@ def rec(tmp_store): # =========================================================================== class TestInit: + """Executor.__init__ wires the injected store/registry through unchanged + and starts a live thread pool.""" def test_stores_injected_dependencies(self, tmp_store, registry): + """Executor keeps references to the exact store and registry instances it was constructed with.""" ex = Executor(store=tmp_store, registry=registry) assert ex.store is tmp_store assert ex.registry is registry def test_default_max_workers(self, tmp_store, registry): + """With no max_workers argument, the executor's thread pool defaults to 8 workers.""" ex = Executor(store=tmp_store, registry=registry) assert ex.pool._max_workers == 8 def test_custom_max_workers(self, tmp_store, registry): + """Passing max_workers sizes the executor's thread pool accordingly.""" ex = Executor(store=tmp_store, registry=registry, max_workers=3) assert ex.pool._max_workers == 3 def test_pool_is_alive(self, executor): + """A freshly constructed executor's thread pool is not shut down.""" assert not executor.pool._shutdown @@ -138,13 +155,18 @@ def test_pool_is_alive(self, executor): # =========================================================================== class TestSubmitHappyPath: + """submit() drives a queued run through RUNNING to COMPLETED, forwarding + inputs/resources to the registered handler and persisting its logs and + results.""" def test_state_transitions_to_running_then_completed(self, executor, tmp_store, rec): + """submit() moves a queued run to COMPLETED once the handler returns successfully.""" executor.submit(rec, {}, {}) final = _wait_for_state(tmp_store, rec.run_id, "COMPLETED") assert final.state == "COMPLETED" def test_results_stored(self, tmp_path, rec): + """The handler's return value is persisted verbatim as the run's results.""" expected = {"score": 42, "items": ["a", "b"]} h = _make_handler(run_fn=lambda i, r, log: expected) reg = _make_registry(h) @@ -156,6 +178,7 @@ def test_results_stored(self, tmp_path, rec): assert final.results == expected def test_logs_contain_start_and_completed(self, executor, tmp_store, rec): + """The run's logs record both a start and a completion message for a successful run.""" executor.submit(rec, {}, {}) final = _wait_for_state(tmp_store, rec.run_id, "COMPLETED") combined = " ".join(final.logs) @@ -163,17 +186,20 @@ def test_logs_contain_start_and_completed(self, executor, tmp_store, rec): assert "Completed" in combined def test_log_contains_tool_id(self, executor, tmp_store, rec): + """The first log line names the tool_id being executed.""" executor.submit(rec, {}, {}) final = _wait_for_state(tmp_store, rec.run_id, "COMPLETED") assert rec.tool_id in final.logs[0] def test_updated_epoch_advances(self, executor, tmp_store, rec): + """updated_epoch does not regress between submission and completion.""" original_epoch = rec.updated_epoch executor.submit(rec, {}, {}) final = _wait_for_state(tmp_store, rec.run_id, "COMPLETED") assert final.updated_epoch >= original_epoch def test_inputs_forwarded_to_handler(self, tmp_path, rec): + """The exact inputs and resources passed to submit() reach the handler's run function unmodified.""" received: list = [] def run_fn(inputs, resources, log): received.append((inputs, resources)) @@ -190,6 +216,7 @@ def run_fn(inputs, resources, log): assert received[0] == (full_inputs, resources) def test_log_callable_passed_to_handler(self, tmp_path, rec): + """The handler receives a callable log() and can invoke it during execution.""" log_calls: list = [] def run_fn(inputs, resources, log): log("custom log line") @@ -205,6 +232,7 @@ def run_fn(inputs, resources, log): assert log_calls # run_fn was called and invoked log() def test_custom_log_line_persisted(self, tmp_path, rec): + """Lines written via the handler's log() callback are persisted in the final run record's logs.""" def run_fn(inputs, resources, log): log("my-custom-line") return {} @@ -218,6 +246,7 @@ def run_fn(inputs, resources, log): assert any("my-custom-line" in line for line in final.logs) def test_error_is_none_on_success(self, executor, tmp_store, rec): + """A successful run leaves the record's error field unset.""" executor.submit(rec, {}, {}) final = _wait_for_state(tmp_store, rec.run_id, "COMPLETED") assert final.error is None @@ -228,6 +257,8 @@ def test_error_is_none_on_success(self, executor, tmp_store, rec): # =========================================================================== class TestSubmitFailurePath: + """submit() catches handler exceptions and terminates the run in FAILED + state with a structured, traceback-bearing error record.""" def _failing_executor(self, tmp_path, exc: Exception): def boom(inputs, resources, log): @@ -241,48 +272,56 @@ def boom(inputs, resources, log): return ex, store, rec def test_state_transitions_to_failed(self, tmp_path): + """submit() moves a run to FAILED when the handler raises.""" ex, store, rec = self._failing_executor(tmp_path, RuntimeError("boom")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert final.state == "FAILED" def test_error_code_is_exec_failed(self, tmp_path): + """A handler exception is recorded with error code EXEC_FAILED.""" ex, store, rec = self._failing_executor(tmp_path, RuntimeError("boom")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert final.error["code"] == "EXEC_FAILED" def test_error_message_contains_exception_text(self, tmp_path): + """The failed run's error message includes the original exception's text.""" ex, store, rec = self._failing_executor(tmp_path, RuntimeError("something went wrong")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert "something went wrong" in final.error["message"] def test_error_trace_is_present(self, tmp_path): + """A failed run's error record includes a non-empty traceback string.""" ex, store, rec = self._failing_executor(tmp_path, ValueError("bad value")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert final.error["trace"] # non-empty traceback string def test_error_trace_contains_exception_type(self, tmp_path): + """The stored traceback names the exception type that was raised.""" ex, store, rec = self._failing_executor(tmp_path, ValueError("bad value")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert "ValueError" in final.error["trace"] def test_failed_log_appended(self, tmp_path): + """A FAILED-state log line is appended when the handler raises.""" ex, store, rec = self._failing_executor(tmp_path, RuntimeError("oops")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert any("FAILED" in line for line in final.logs) def test_results_is_none_on_failure(self, tmp_path): + """A failed run leaves results unset (None).""" ex, store, rec = self._failing_executor(tmp_path, RuntimeError("oops")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") assert final.results is None def test_updated_epoch_set_on_failure(self, tmp_path): + """updated_epoch does not regress when a run fails.""" ex, store, rec = self._failing_executor(tmp_path, RuntimeError("oops")) original = rec.updated_epoch ex.submit(rec, {}, {}) @@ -290,6 +329,7 @@ def test_updated_epoch_set_on_failure(self, tmp_path): assert final.updated_epoch >= original def test_non_runtime_exception_still_fails(self, tmp_path): + """Non-RuntimeError exceptions (e.g. KeyError) from the handler also fail the run rather than propagating or being swallowed.""" ex, store, rec = self._failing_executor(tmp_path, KeyError("missing key")) ex.submit(rec, {}, {}) final = _wait_for_state(store, rec.run_id, "FAILED") @@ -301,8 +341,12 @@ def test_non_runtime_exception_still_fails(self, tmp_path): # =========================================================================== class TestSubmitRegistryLookup: + """submit() resolves the handler by the run's tool_id via the injected + registry, and rejects runs for unregistered tools before any work + starts.""" def test_uses_handler_for_correct_tool_id(self, tmp_path): + """submit() dispatches to the handler registered under the run's own tool_id, not any other registered handler.""" called_with: list = [] def run_fn(inputs, resources, log): called_with.append(inputs) @@ -320,6 +364,7 @@ def run_fn(inputs, resources, log): assert called_with == [{"x": 1}] def test_unregistered_tool_raises_at_submit_time(self, tmp_store): + """submit() raises KeyError naming the tool_id immediately when no handler is registered for it, rather than failing the run asynchronously.""" reg = ToolRegistry() # empty — nothing registered ex = Executor(store=tmp_store, registry=reg) rec = _make_record(tool_id="ghost_tool") @@ -333,8 +378,11 @@ def test_unregistered_tool_raises_at_submit_time(self, tmp_store): # =========================================================================== class TestAppendLog: + """The handler's log callback appends lines to the run record and keeps + updated_epoch fresh as work progresses.""" def test_multiple_log_lines_all_persisted(self, tmp_path, rec): + """Every line written via log() during a run is retained in the final record's logs, in order.""" def run_fn(inputs, resources, log): for i in range(5): log(f"line-{i}") @@ -351,6 +399,7 @@ def run_fn(inputs, resources, log): assert f"line-{i}" in log_text def test_append_log_updates_epoch(self, tmp_path, rec): + """Calling log() during a run updates the record's updated_epoch, observable mid-execution.""" epoch_snapshots: list = [] def run_fn(inputs, resources, log): @@ -372,8 +421,12 @@ def run_fn(inputs, resources, log): # =========================================================================== class TestConcurrency: + """The thread-pool-backed executor runs multiple submitted jobs + concurrently, and one job's failure does not affect the outcome of + independent jobs.""" def test_multiple_jobs_all_complete(self, tmp_path): + """Submitting many runs to a multi-worker executor lets them all complete independently and concurrently.""" n = 10 store = _make_store(tmp_path) records = [_make_record(run_id=f"run-{i}") for i in range(n)] @@ -393,6 +446,7 @@ def test_multiple_jobs_all_complete(self, tmp_path): assert store.get(r.run_id).state == "COMPLETED" def test_failed_job_does_not_affect_others(self, tmp_path): + """One submitted run failing does not prevent a concurrently submitted, unrelated run from completing successfully.""" store = _make_store(tmp_path) good_rec = _make_record(run_id="good") bad_rec = _make_record(run_id="bad") diff --git a/tests/test_http_tool_executor.py b/tests/test_http_tool_executor.py index f441efb..a64865a 100644 --- a/tests/test_http_tool_executor.py +++ b/tests/test_http_tool_executor.py @@ -1,3 +1,17 @@ +"""Tests for toolserver/adapters/http_tool_executor.py. + +Covers the generic HTTP-based tool adapter that ToolServer uses to invoke +externally-defined "http" tools from tool_def configuration: placeholder +substitution (_resolve), dot/array-index response-path extraction +(_get_nested), input validation (make_validate), and request execution +(make_run) -- URL/param/body construction, GET/POST dispatch, response +parsing (including the non-JSON fallback), secret-field log masking, and +the config-error guard clauses that reject malformed tool definitions. + +Developer: + Manish Kumar +""" + from __future__ import annotations import pytest @@ -16,10 +30,15 @@ # ═════════════════════════════════════════════════════════════════════════════ class TestResolve: + """Substitute {param} placeholders in a template string with resolved + input values, leaving unmatched placeholders untouched.""" + def test_single_placeholder(self): + """A single {name} placeholder is substituted with its input value.""" assert _resolve("Hello, {name}!", {"name": "World"}) == "Hello, World!" def test_multiple_placeholders(self): + """Multiple distinct placeholders in one template are all substituted.""" result = _resolve("{method} {resource} v{version}", { "method": "GET", "resource": "genes", "version": 2 }) @@ -30,15 +49,20 @@ def test_missing_key_leaves_placeholder(self): assert _resolve("Hello, {name}!", {}) == "Hello, {name}!" def test_integer_value_is_stringified(self): + """An integer input value is stringified when substituted into the template.""" assert _resolve("/page/{page}", {"page": 3}) == "/page/3" def test_no_placeholders(self): + """A template with no placeholders is returned unchanged.""" assert _resolve("/api/v1/health", {}) == "/api/v1/health" def test_empty_template(self): + """An empty template string resolves to an empty string.""" assert _resolve("", {"key": "val"}) == "" def test_partial_substitution(self): + """Placeholders with a matching input are substituted; placeholders + without a matching key are left as-is.""" result = _resolve("{a}/{b}/{c}", {"a": "x", "c": "z"}) assert result == "x/{b}/z" @@ -48,40 +72,54 @@ def test_partial_substitution(self): # ═════════════════════════════════════════════════════════════════════════════ class TestGetNested: + """Resolve dot-notation and array-index paths (e.g. "results[0].name") + against parsed response JSON, returning None for any path segment that + doesn't exist rather than raising.""" + def test_empty_path_returns_data(self): + """An empty path returns the input data object itself, unchanged.""" data = {"key": "value"} assert _get_nested(data, "") is data def test_simple_key(self): + """A single top-level dict key is resolved directly.""" assert _get_nested({"name": "gene1"}, "name") == "gene1" def test_nested_dot_path(self): + """Dot-separated keys traverse nested dicts.""" data = {"organism": {"scientificName": "Homo sapiens"}} assert _get_nested(data, "organism.scientificName") == "Homo sapiens" def test_array_index(self): + """A bracketed index selects an element from a list.""" data = {"results": ["a", "b", "c"]} assert _get_nested(data, "results[1]") == "b" def test_array_index_with_nested_key(self): + """An array index can be followed by a dotted key into the selected element.""" data = {"results": [{"name": "gene1"}, {"name": "gene2"}]} assert _get_nested(data, "results[0].name") == "gene1" def test_out_of_bounds_index_returns_none(self): + """An out-of-range array index returns None instead of raising IndexError.""" data = {"results": ["only_one"]} assert _get_nested(data, "results[5]") is None def test_missing_dict_key_returns_none(self): + """A dict key that isn't present returns None instead of raising KeyError.""" assert _get_nested({"a": 1}, "b") is None def test_deeply_nested(self): + """Multiple chained dot-separated keys resolve through several nesting levels.""" data = {"a": {"b": {"c": {"d": 42}}}} assert _get_nested(data, "a.b.c.d") == 42 def test_path_on_non_container_returns_none(self): + """Applying a path to a non-dict, non-list value returns None.""" assert _get_nested("just_a_string", "key") is None def test_none_data_with_path_returns_none(self): + """Applying a non-empty path to None data returns None instead of raising.""" assert _get_nested(None, "key") is None @@ -101,44 +139,57 @@ def test_none_data_with_path_returns_none(self): class TestMakeValidate: + """Validate tool inputs against a tool_def's declared fields: required-ness, + blank-string rejection for required strings, and integer/number type checks.""" + @pytest.fixture def validate(self): return make_validate(TOOL_DEF_VALIDATE) def test_valid_inputs_pass(self, validate): + """Inputs satisfying all required/type constraints validate with no errors.""" result = validate({"query": "BRCA1", "limit": 5}, {}) assert result["ok"] is True assert result["errors"] == [] def test_missing_required_field_fails(self, validate): + """Omitting a required field fails validation with an error naming that field.""" result = validate({}, {}) assert result["ok"] is False assert any(e["field"] == "query" for e in result["errors"]) def test_blank_string_for_required_fails(self, validate): + """A required string field containing only whitespace fails validation + as if the field were missing.""" result = validate({"query": " "}, {}) assert result["ok"] is False def test_wrong_type_integer_fails(self, validate): + """A non-integer value for an integer-typed field fails validation + with an error naming that field.""" result = validate({"query": "BRCA1", "limit": "not_an_int"}, {}) assert result["ok"] is False assert any(e["field"] == "limit" for e in result["errors"]) def test_wrong_type_number_fails(self, validate): + """A non-numeric value for a number-typed field fails validation + with an error naming that field.""" result = validate({"query": "BRCA1", "score": "high"}, {}) assert result["ok"] is False assert any(e["field"] == "score" for e in result["errors"]) def test_integer_accepted_for_number_field(self, validate): - """int is a valid number.""" + """An integer value is accepted for a number-typed field.""" result = validate({"query": "BRCA1", "score": 7}, {}) assert result["ok"] is True def test_float_accepted_for_number_field(self, validate): + """A float value is accepted for a number-typed field.""" result = validate({"query": "BRCA1", "score": 3.14}, {}) assert result["ok"] is True def test_optional_field_missing_is_ok(self, validate): + """Omitting a non-required field with no default still validates successfully.""" result = validate({"query": "BRCA1"}, {}) assert result["ok"] is True @@ -148,10 +199,12 @@ def test_default_satisfies_optional_field(self, validate): assert result["ok"] is True def test_no_inputs_defined(self): + """A tool_def with an empty inputs list validates any input dict as OK.""" validate = make_validate({"tool_id": "empty", "inputs": []}) assert validate({}, {})["ok"] is True def test_warnings_always_empty(self, validate): + """Validation never populates the warnings list.""" result = validate({"query": "test"}, {}) assert result["warnings"] == [] @@ -209,6 +262,10 @@ def _make_mock_response(json_data: Any, status_code: int = 200) -> MagicMock: # ═════════════════════════════════════════════════════════════════════════════ class TestMakeRunGet: + """Execute a GET-method tool: build the resolved URL/params, apply input + defaults, mask secret fields in the log line, and map the raw response + through response_map.""" + @pytest.fixture def run(self): return make_run(BASE_TOOL_DEF) @@ -222,6 +279,8 @@ def _patch_get(self, json_data): return patch("httpx.Client.get", return_value=mock_resp) def test_get_returns_raw_and_mapped(self, run, log): + """The parsed response is returned unmapped under 'raw', and also + mapped through response_map into the tool's own named output fields.""" json_data = { "results": [{"name": "BRCA1"}], "metadata": {"total": 42}, @@ -234,6 +293,8 @@ def test_get_returns_raw_and_mapped(self, run, log): assert result["total"] == 42 def test_get_url_placeholder_resolved(self, run, log): + """A {placeholder} embedded in the URL template is resolved from the + validated input values before the GET request is issued.""" tool_def = { **BASE_TOOL_DEF, "http": { @@ -252,6 +313,8 @@ def test_get_url_placeholder_resolved(self, run, log): assert "TP53" in called_url def test_default_values_applied(self, run, log): + """An omitted optional input falls back to its declared default when + the request params are built.""" json_data = {"results": [], "metadata": {"total": 0}} mock_resp = _make_mock_response(json_data) @@ -261,11 +324,15 @@ def test_default_values_applied(self, run, log): assert kwargs["params"]["limit"] == 10 def test_log_called_twice(self, run, log): + """A successful run logs exactly twice: once for the outgoing + request and once for the received response.""" with self._patch_get({"results": [], "metadata": {}}): run({"query": "X"}, {}, log) assert log.call_count == 2 def test_log_masks_secret_fields(self, log): + """An input field flagged secret=True has its value replaced with + '***' in the log line, so it never appears in plaintext in logs.""" tool_def = { **BASE_TOOL_DEF, "inputs": [ @@ -282,12 +349,16 @@ def test_log_masks_secret_fields(self, log): assert "***" in first_log_call def test_raise_for_status_called(self, run, log): + """The response's raise_for_status() is always invoked, so a non-2xx + HTTP status is rejected rather than treated as a successful result.""" mock_resp = _make_mock_response({}) with patch("httpx.Client.get", return_value=mock_resp): run({"query": "X"}, {}, log) mock_resp.raise_for_status.assert_called_once() def test_http_error_propagates(self, run, log): + """An HTTPStatusError raised by raise_for_status() propagates out of + run() to the caller rather than being swallowed.""" mock_resp = _make_mock_response({}, status_code=404) mock_resp.raise_for_status.side_effect = httpx.HTTPStatusError( "Not Found", request=MagicMock(), response=mock_resp @@ -334,11 +405,17 @@ def test_response_map_missing_path_returns_none(self, run, log): class TestMakeRunPost: + """Execute a POST-method tool: build the request body via body_map + {placeholder} substitution, honor body_type (json/form) when choosing + how to send it, and map the response through response_map.""" + @pytest.fixture def log(self): return MagicMock() def test_post_json_body_sent(self, log): + """body_type 'json' sends the body_map-substituted body as the + request's JSON payload, and the response is mapped as usual.""" run = make_run(POST_TOOL_DEF) mock_resp = _make_mock_response({"id": "job-123"}) @@ -350,6 +427,8 @@ def test_post_json_body_sent(self, log): assert result["job_id"] == "job-123" def test_post_form_body_sent(self, log): + """body_type 'form' sends the body_map-substituted body via the + form-encoded 'data' kwarg instead of 'json'.""" run = make_run(POST_FORM_TOOL_DEF) mock_resp = _make_mock_response({"id": "job-456"}) @@ -360,6 +439,8 @@ def test_post_form_body_sent(self, log): assert kwargs["data"]["seq"] == "GCTA" def test_post_default_body_type_is_json(self, log): + """Omitting body_type from the http block defaults the POST body + encoding to JSON.""" tool_def = { **POST_TOOL_DEF, "http": {k: v for k, v in POST_TOOL_DEF["http"].items() if k != "body_type"}, @@ -378,7 +459,12 @@ def test_post_default_body_type_is_json(self, log): # ═════════════════════════════════════════════════════════════════════════════ class TestMakeRunUnsupportedMethod: + """An http.method outside the supported GET/POST set is rejected before + any request is attempted, rather than silently falling through.""" + def test_unsupported_method_raises(self): + """A DELETE (or any unsupported) HTTP method raises ValueError + naming the unsupported method, before any request is sent.""" tool_def = { **BASE_TOOL_DEF, "http": {**BASE_TOOL_DEF["http"], "method": "DELETE"}, @@ -393,7 +479,12 @@ def test_unsupported_method_raises(self): # ═════════════════════════════════════════════════════════════════════════════ class TestMakeRunDefaultMethod: + """Omitting 'method' from the http block defaults the request to GET + rather than raising or refusing to run.""" + def test_default_method_is_get(self): + """With no 'method' key in the http block, the run still dispatches + the request via httpx.Client.get.""" tool_def = { **BASE_TOOL_DEF, "http": {k: v for k, v in BASE_TOOL_DEF["http"].items() if k != "method"}, @@ -411,6 +502,10 @@ def test_default_method_is_get(self): # ═════════════════════════════════════════════════════════════════════════════ class TestParseResponse: + """Parse an httpx Response body into a dict: an application/json + content-type or a response whose body still parses as JSON returns the + parsed JSON; a genuine JSON-parse failure falls back to raw text.""" + def test_json_content_type_returns_parsed_json(self): """Line 60: content-type is application/json → early return via resp.json().""" mock_resp = MagicMock() @@ -440,6 +535,9 @@ def test_json_parse_failure_returns_text(self): # ═════════════════════════════════════════════════════════════════════════════ class TestMakeRunConfigErrors: + """A malformed or absent 'http' block in a tool_def is rejected at + run-time with ValueError, before any network request is attempted.""" + def test_raises_when_http_block_absent(self): """Line 104: tool_def has no 'http' key → ValueError.""" run = make_run({"tool_id": "no_http"}) @@ -470,6 +568,10 @@ def test_raises_when_url_missing_from_http_block(self): # ═════════════════════════════════════════════════════════════════════════════ class TestMakeRunIntegerParamFallback: + """An integer-typed param whose input value fails int() conversion is + not dropped or rejected -- it's kept in the request as its original + string value.""" + def test_non_numeric_integer_param_kept_as_string(self): """Lines 134-135: int() conversion fails → param value kept as string.""" tool_def = { diff --git a/tests/test_store.py b/tests/test_store.py index 9003c11..9a60d1d 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -1,8 +1,14 @@ """ -Unit tests for toolserver.store.RunStore. +Tests for toolserver.store.RunStore, the JSON-file-backed persistence layer +for run records. -Run with: - python -m pytest tests/test_store.py -v +Covers construction/directory creation, path derivation, create/get/update/ +try_get behavior against both the in-memory cache and disk, the atomic +(write-tmp-then-os.replace) write path used by update(), and thread safety +under concurrent creates/updates/reads via the store's shared lock. + +Developer: + Manish Kumar """ from __future__ import annotations @@ -59,22 +65,31 @@ def rec() -> RunRecord: # =========================================================================== class TestInit: + """RunStore's constructor creates its storage directory as needed and + starts with a clean in-memory cache and lock.""" def test_creates_directory_if_missing(self, tmp_path: Path) -> None: + """Constructing a RunStore over a non-existent nested path creates + that directory tree.""" target = tmp_path / "deep" / "nested" / "store" assert not target.exists() RunStore(root_dir=str(target)) assert target.is_dir() def test_accepts_existing_directory(self, tmp_path: Path) -> None: + """Constructing a RunStore over an already-existing directory does + not raise.""" # Must not raise if the directory already exists RunStore(root_dir=str(tmp_path)) RunStore(root_dir=str(tmp_path)) def test_cache_starts_empty(self, store: RunStore) -> None: + """A freshly constructed RunStore's in-memory cache is empty.""" assert store._cache == {} def test_lock_is_created(self, store: RunStore) -> None: + """A freshly constructed RunStore has a lock ready for + synchronized cache/file access.""" assert store._lock is not None @@ -83,13 +98,18 @@ def test_lock_is_created(self, store: RunStore) -> None: # =========================================================================== class TestPath: + """RunStore._path() deterministically derives each run's on-disk JSON + file location from its run_id.""" def test_returns_json_file_inside_root(self, store: RunStore) -> None: + """_path() returns a ".json" file located directly inside + the store's root directory.""" p = store._path("abc-123") assert p.parent == store.root assert p.name == "abc-123.json" def test_different_ids_give_different_paths(self, store: RunStore) -> None: + """_path() maps distinct run_ids to distinct file paths.""" assert store._path("id-1") != store._path("id-2") @@ -98,23 +118,32 @@ def test_different_ids_give_different_paths(self, store: RunStore) -> None: # =========================================================================== class TestCreate: + """RunStore.create() persists a new run record to both the in-memory + cache and a JSON file on disk.""" def test_file_written(self, store: RunStore, rec: RunRecord) -> None: + """create() writes a JSON file for the run at its derived path.""" store.create(rec) assert store._path(rec.run_id).exists() def test_file_contains_valid_json(self, store: RunStore, rec: RunRecord) -> None: + """create() writes valid JSON whose run_id field matches the + record.""" store.create(rec) raw = store._path(rec.run_id).read_text() data = json.loads(raw) assert data["run_id"] == rec.run_id def test_record_added_to_cache(self, store: RunStore, rec: RunRecord) -> None: + """create() stores the exact same record object in the in-memory + cache, keyed by run_id.""" store.create(rec) assert rec.run_id in store._cache assert store._cache[rec.run_id] is rec def test_overwrite_existing_record(self, store: RunStore, rec: RunRecord) -> None: + """create() called again with the same run_id overwrites both the + cached record and the on-disk file with the new state.""" store.create(rec) updated = _make_record(run_id=rec.run_id, state="RUNNING") store.create(updated) @@ -123,6 +152,8 @@ def test_overwrite_existing_record(self, store: RunStore, rec: RunRecord) -> Non assert data["state"] == "RUNNING" def test_multiple_records_written(self, store: RunStore) -> None: + """create() called for several distinct run_ids writes a separate + file and cache entry for each.""" recs = [_make_record(run_id=f"run-{i}") for i in range(5)] for r in recs: store.create(r) @@ -136,34 +167,49 @@ def test_multiple_records_written(self, store: RunStore) -> None: # =========================================================================== class TestGet: + """RunStore.get() serves cached records without touching disk, falls + back to loading and caching from disk on a cache miss, and raises + KeyError naming the missing run_id when no record exists anywhere.""" def test_returns_cached_record(self, store: RunStore, rec: RunRecord) -> None: + """get() returns the identical cached object, not a reloaded + copy, when the run_id is already in cache.""" store.create(rec) result = store.get(rec.run_id) assert result is rec # exact same object from cache def test_loads_from_disk_when_not_in_cache(self, store: RunStore, rec: RunRecord) -> None: + """get() falls back to reading the on-disk file when the run_id + has been evicted from the cache.""" store.create(rec) store._cache.clear() # evict from cache result = store.get(rec.run_id) assert result.run_id == rec.run_id def test_disk_load_populates_cache(self, store: RunStore, rec: RunRecord) -> None: + """get() re-populates the cache after loading a record from + disk.""" store.create(rec) store._cache.clear() store.get(rec.run_id) assert rec.run_id in store._cache def test_raises_key_error_for_missing_id(self, store: RunStore) -> None: + """get() raises KeyError with a "run not found" message for a + run_id with no cache entry or file.""" with pytest.raises(KeyError, match="run not found"): store.get("does-not-exist") def test_key_error_message_contains_run_id(self, store: RunStore) -> None: + """get()'s KeyError message includes the specific missing + run_id, not just a generic message.""" missing_id = "ghost-run-999" with pytest.raises(KeyError, match=missing_id): store.get(missing_id) def test_round_trip_preserves_fields(self, store: RunStore) -> None: + """A record's fields (state, inputs, etc.) survive a + create()-then-disk-load round trip unchanged.""" rec = _make_record(run_id="rt-1", state="COMPLETED", inputs={"genes": ["MYC"]}) store.create(rec) store._cache.clear() @@ -175,6 +221,8 @@ def test_round_trip_preserves_fields(self, store: RunStore) -> None: def test_disk_file_with_extra_whitespace_still_loads( self, store: RunStore, rec: RunRecord ) -> None: + """get() parses the indented, multi-line JSON produced by + model_dump_json(indent=2) without error.""" store.create(rec) store._cache.clear() # model_dump_json with indent=2 produces multi-line JSON; make sure it parses fine @@ -189,14 +237,21 @@ def test_disk_file_with_extra_whitespace_still_loads( # =========================================================================== class TestUpdate: + """RunStore.update() writes through a temp file and a single + os.replace() so the on-disk record is never left partially written, + and the change is immediately visible via the cache.""" def test_cache_updated(self, store: RunStore, rec: RunRecord) -> None: + """update() replaces the cached record for a run_id with the new + state.""" store.create(rec) updated = _make_record(run_id=rec.run_id, state="COMPLETED") store.update(updated) assert store._cache[rec.run_id].state == "COMPLETED" def test_file_updated_on_disk(self, store: RunStore, rec: RunRecord) -> None: + """update() overwrites the on-disk JSON file with the new record + state.""" store.create(rec) updated = _make_record(run_id=rec.run_id, state="COMPLETED") store.update(updated) @@ -204,12 +259,17 @@ def test_file_updated_on_disk(self, store: RunStore, rec: RunRecord) -> None: assert data["state"] == "COMPLETED" def test_no_tmp_file_left_behind(self, store: RunStore, rec: RunRecord) -> None: + """update() leaves no "*.json.tmp" file behind after a + successful write.""" store.create(rec) store.update(_make_record(run_id=rec.run_id, state="COMPLETED")) tmp = store._path(rec.run_id).with_suffix(".json.tmp") assert not tmp.exists() def test_atomic_replace_used(self, store: RunStore, rec: RunRecord) -> None: + """update() writes to a .tmp file first and swaps it into place + via a single os.replace() call, rather than writing the target + file directly.""" store.create(rec) replaced: list[str] = [] @@ -227,6 +287,8 @@ def test_atomic_replace_used(self, store: RunStore, rec: RunRecord) -> None: assert dst.endswith(f"{rec.run_id}.json") def test_update_without_prior_create(self, store: RunStore) -> None: + """update() writes a new record to cache and disk even when + create() was never called for that run_id first.""" # update() makes no precondition check — it should still write rec = _make_record(run_id="new-run", state="RUNNING") store.update(rec) @@ -234,6 +296,8 @@ def test_update_without_prior_create(self, store: RunStore) -> None: assert store._cache["new-run"].state == "RUNNING" def test_get_after_update_returns_new_state(self, store: RunStore, rec: RunRecord) -> None: + """get() immediately reflects a record's updated state after + update(), via the cache.""" store.create(rec) store.update(_make_record(run_id=rec.run_id, state="COMPLETED")) # get() should return the updated record from cache @@ -246,18 +310,24 @@ def test_get_after_update_returns_new_state(self, store: RunStore, rec: RunRecor # =========================================================================== class TestTryGet: + """RunStore.try_get() wraps get() to return None instead of raising + KeyError for a missing run_id.""" def test_returns_record_when_found(self, store: RunStore, rec: RunRecord) -> None: + """try_get() returns the record when the run_id exists.""" store.create(rec) result = store.try_get(rec.run_id) assert result is not None assert result.run_id == rec.run_id def test_returns_none_when_missing(self, store: RunStore) -> None: + """try_get() returns None, not an exception, for a run_id that + doesn't exist.""" result = store.try_get("ghost-999") assert result is None def test_does_not_raise_for_missing(self, store: RunStore) -> None: + """try_get() never raises for a missing run_id.""" # Must never raise — the whole point of try_get try: store.try_get("does-not-exist") @@ -265,6 +335,8 @@ def test_does_not_raise_for_missing(self, store: RunStore) -> None: pytest.fail(f"try_get raised unexpectedly: {exc}") def test_delegates_to_get(self, store: RunStore, rec: RunRecord) -> None: + """try_get() is implemented in terms of get(), calling it exactly + once with the given run_id.""" store.create(rec) with patch.object(store, "get", wraps=store.get) as mock_get: store.try_get(rec.run_id) @@ -276,8 +348,14 @@ def test_delegates_to_get(self, store: RunStore, rec: RunRecord) -> None: # =========================================================================== class TestThreadSafety: + """Concurrent creates, updates, and reads against RunStore never + raise, lose records, or corrupt on-disk JSON, because all cache/file + access is serialized through the store's shared lock.""" def test_concurrent_creates_all_succeed(self, store: RunStore) -> None: + """20 threads calling create() concurrently for distinct run_ids + all succeed without error, and every record ends up in the + cache.""" errors: List[Exception] = [] def create_one(i: int) -> None: @@ -296,6 +374,9 @@ def create_one(i: int) -> None: assert len(store._cache) == 20 def test_concurrent_updates_do_not_corrupt_file(self, store: RunStore) -> None: + """20 threads calling update() concurrently on the same run_id + never raise, and the file on disk is always well-formed, + parseable JSON afterward.""" rec = _make_record(run_id="shared-run") store.create(rec) errors: List[Exception] = [] @@ -323,6 +404,8 @@ def update_one(i: int) -> None: assert data["run_id"] == "shared-run" def test_concurrent_get_and_create(self, store: RunStore) -> None: + """Concurrent readers calling get() and writers calling update() + on the same run_id never raise, regardless of interleaving.""" rec = _make_record(run_id="race-run") store.create(rec) errors: List[Exception] = [] diff --git a/tests/test_tools_init.py b/tests/test_tools_init.py index 661601e..eded093 100644 --- a/tests/test_tools_init.py +++ b/tests/test_tools_init.py @@ -1,3 +1,15 @@ +"""Tests for toolserver/tools/__init__.py, the tool registry bootstrap. + +Covers register_tools's registration of the two built-in tools +(enrichr_pathway, david_annotation) and load_tools_from_yaml's parsing of +a YAML tool catalog into registered HTTP-tool handlers, including its +file-not-found handling and its rules for skipping catalog entries that +are missing a tool_id or an http block. + +Developer: + Manish Kumar +""" + from __future__ import annotations import textwrap @@ -33,7 +45,10 @@ def _write_yaml(tmp_path: Path, data) -> Path: # ═════════════════════════════════════════════════════════════════════════════ class TestRegisterTools: + """register_tools's registration of the two built-in tool handlers.""" + def test_registers_enrichr_pathway(self): + """The built-in enrichr_pathway tool is registered under that tool_id.""" registry = _make_registry() register_tools(registry) @@ -41,6 +56,7 @@ def test_registers_enrichr_pathway(self): assert "enrichr_pathway" in ids def test_registers_david_annotation(self): + """The built-in david_annotation tool is registered under that tool_id.""" registry = _make_registry() register_tools(registry) @@ -48,17 +64,20 @@ def test_registers_david_annotation(self): assert "david_annotation" in ids def test_registers_both_builtin_tools(self): + """Exactly the two built-in tools are registered, no more and no fewer.""" registry = _make_registry() register_tools(registry) assert registry.register.call_count == 2 def test_enrichr_pathway_has_correct_version(self): + """The enrichr_pathway handler is registered with version v1.""" registry = _make_registry() register_tools(registry) assert registry.registered[0].version == "v1" def test_enrichr_pathway_has_default_libraries_feature(self): + """The enrichr_pathway handler's default feature set includes its default libraries.""" registry = _make_registry() register_tools(registry) features = registry.registered[0].features @@ -67,6 +86,7 @@ def test_enrichr_pathway_has_default_libraries_feature(self): assert "Reactome_2022" in features["libraries_default"] def test_enrichr_pathway_validate_and_run_are_callable(self): + """The registered enrichr_pathway handler exposes callable validate and run hooks.""" registry = _make_registry() register_tools(registry) handler = registry.registered[0] @@ -79,12 +99,16 @@ def test_enrichr_pathway_validate_and_run_are_callable(self): # ═════════════════════════════════════════════════════════════════════════════ class TestLoadToolsFromYamlFileNotFound: + """load_tools_from_yaml's error handling when the YAML catalog file is missing.""" + def test_raises_file_not_found(self, tmp_path): + """A missing YAML catalog path raises FileNotFoundError with a descriptive message.""" registry = _make_registry() with pytest.raises(FileNotFoundError, match="tools YAML not found"): load_tools_from_yaml(registry, str(tmp_path / "missing.yaml")) def test_error_message_contains_path(self, tmp_path): + """The FileNotFoundError message includes the specific missing file's path.""" registry = _make_registry() bad_path = str(tmp_path / "no_such_file.yaml") with pytest.raises(FileNotFoundError, match="no_such_file.yaml"): @@ -111,7 +135,10 @@ def test_error_message_contains_path(self, tmp_path): class TestLoadToolsFromYamlHappyPath: + """load_tools_from_yaml's registration of well-formed HTTP-tool catalog entries.""" + def test_registers_single_http_tool(self, tmp_path): + """A single well-formed HTTP-tool entry is registered under its tool_id.""" registry = _make_registry() p = _write_yaml(tmp_path, [MINIMAL_HTTP_TOOL]) load_tools_from_yaml(registry, str(p)) @@ -120,18 +147,21 @@ def test_registers_single_http_tool(self, tmp_path): assert registry.registered[0].tool_id == "gene_search" def test_registered_handler_version(self, tmp_path): + """The registered handler's version matches the catalog entry's version field.""" registry = _make_registry() p = _write_yaml(tmp_path, [MINIMAL_HTTP_TOOL]) load_tools_from_yaml(registry, str(p)) assert registry.registered[0].version == "v2" def test_registered_handler_features(self, tmp_path): + """The registered handler's features match the catalog entry's features field.""" registry = _make_registry() p = _write_yaml(tmp_path, [MINIMAL_HTTP_TOOL]) load_tools_from_yaml(registry, str(p)) assert registry.registered[0].features == {"max_results": 100} def test_validate_and_run_are_callable(self, tmp_path): + """A handler built from a YAML entry exposes callable validate and run hooks.""" registry = _make_registry() p = _write_yaml(tmp_path, [MINIMAL_HTTP_TOOL]) load_tools_from_yaml(registry, str(p)) @@ -140,6 +170,7 @@ def test_validate_and_run_are_callable(self, tmp_path): assert callable(handler.run) def test_registers_multiple_http_tools(self, tmp_path): + """Multiple HTTP-tool entries are all registered, preserving catalog order.""" tools = [ {**MINIMAL_HTTP_TOOL, "tool_id": "tool_a"}, {**MINIMAL_HTTP_TOOL, "tool_id": "tool_b"}, @@ -153,6 +184,7 @@ def test_registers_multiple_http_tools(self, tmp_path): assert ids == ["tool_a", "tool_b", "tool_c"] def test_default_version_when_not_specified(self, tmp_path): + """An entry with no version field defaults to v1.""" tool = {k: v for k, v in MINIMAL_HTTP_TOOL.items() if k != "version"} registry = _make_registry() p = _write_yaml(tmp_path, [tool]) @@ -160,6 +192,7 @@ def test_default_version_when_not_specified(self, tmp_path): assert registry.registered[0].version == "v1" def test_default_features_when_not_specified(self, tmp_path): + """An entry with no features field defaults to an empty features dict.""" tool = {k: v for k, v in MINIMAL_HTTP_TOOL.items() if k != "features"} registry = _make_registry() p = _write_yaml(tmp_path, [tool]) @@ -167,6 +200,7 @@ def test_default_features_when_not_specified(self, tmp_path): assert registry.registered[0].features == {} def test_prints_loaded_count(self, tmp_path, capsys): + """Loading the catalog prints a summary line with the number of HTTP tools loaded.""" p = _write_yaml(tmp_path, [MINIMAL_HTTP_TOOL]) load_tools_from_yaml(_make_registry(), str(p)) out = capsys.readouterr().out @@ -179,7 +213,10 @@ def test_prints_loaded_count(self, tmp_path, capsys): # ═════════════════════════════════════════════════════════════════════════════ class TestLoadToolsFromYamlSkipping: + """load_tools_from_yaml's rules for skipping malformed or non-HTTP catalog entries.""" + def test_skips_tool_without_http_block(self, tmp_path): + """An entry with no http block is skipped; entries with one are still registered.""" tools = [ {"tool_id": "legacy_tool", "inputs": []}, # no 'http' key MINIMAL_HTTP_TOOL, @@ -192,6 +229,7 @@ def test_skips_tool_without_http_block(self, tmp_path): assert registry.registered[0].tool_id == "gene_search" def test_skips_tool_without_tool_id(self, tmp_path, capsys): + """An entry missing tool_id is skipped and a warning is printed, without aborting the load.""" tools = [ {"http": MINIMAL_HTTP_TOOL["http"], "inputs": []}, # missing tool_id MINIMAL_HTTP_TOOL, @@ -207,6 +245,7 @@ def test_skips_tool_without_tool_id(self, tmp_path, capsys): assert "WARNING" in out def test_empty_yaml_registers_nothing(self, tmp_path): + """An empty YAML file (parses to None) registers no tools and does not raise.""" p = tmp_path / "tools.yaml" p.write_text("") # empty file → yaml.safe_load returns None registry = _make_registry() @@ -214,6 +253,7 @@ def test_empty_yaml_registers_nothing(self, tmp_path): registry.register.assert_not_called() def test_yaml_with_only_non_http_tools_registers_nothing(self, tmp_path): + """A catalog containing only entries without an http block registers nothing.""" tools = [ {"tool_id": "a", "inputs": []}, {"tool_id": "b", "inputs": []}, @@ -224,6 +264,7 @@ def test_yaml_with_only_non_http_tools_registers_nothing(self, tmp_path): registry.register.assert_not_called() def test_mixed_tools_only_http_ones_registered(self, tmp_path): + """In a mixed catalog, only the HTTP-tool entries are registered, in their original order.""" tools = [ {"tool_id": "legacy"}, # no http {**MINIMAL_HTTP_TOOL, "tool_id": "http_1"}, @@ -234,4 +275,4 @@ def test_mixed_tools_only_http_ones_registered(self, tmp_path): p = _write_yaml(tmp_path, tools) load_tools_from_yaml(registry, str(p)) ids = [h.tool_id for h in registry.registered] - assert ids == ["http_1", "http_2"] \ No newline at end of file + assert ids == ["http_1", "http_2"] diff --git a/tests/test_toolserver_app_coverage.py b/tests/test_toolserver_app_coverage.py index 75cce62..bf0cc52 100644 --- a/tests/test_toolserver_app_coverage.py +++ b/tests/test_toolserver_app_coverage.py @@ -1,4 +1,11 @@ # tests/test_toolserver_app_coverage.py +""" +Coverage-focused tests for toolserver_app.py's app-creation branches +(tools YAML present/absent) and the disabled /register_tools endpoint. + +Developer: + Manish Kumar +""" import pytest import yaml from fastapi.testclient import TestClient @@ -41,6 +48,7 @@ def test_create_app_yaml_not_found(capsys): # registration code paths they used to cover are now unreachable by # design, not merely untested. def test_register_tools_disabled_returns_501(): + """Reject a well-formed HTTP-tool registration with 501 and the REGISTER_TOOLS_AUTHORIZATION_MODEL_UNRESOLVED code.""" from toolserver_app import create_app client = TestClient(create_app()) @@ -58,6 +66,7 @@ def test_register_tools_disabled_returns_501(): def test_register_tools_disabled_regardless_of_payload_shape(): + """Reject a minimal/stub-shaped registration payload with 501 just like a full one.""" from toolserver_app import create_app client = TestClient(create_app()) @@ -66,6 +75,7 @@ def test_register_tools_disabled_regardless_of_payload_shape(): def test_register_tools_disabled_for_empty_tools_list(): + """Reject registration with 501 even when the tools list is empty.""" from toolserver_app import create_app client = TestClient(create_app()) diff --git a/tests/test_toolserver_delegated_auth.py b/tests/test_toolserver_delegated_auth.py index 11b1ecc..0b5a144 100644 --- a/tests/test_toolserver_delegated_auth.py +++ b/tests/test_toolserver_delegated_auth.py @@ -15,6 +15,9 @@ covered by omnibioai-iam-client's own 41-test delegated-identity suite and is not re-tested here. One deeper test below additionally mocks the raw httpx call to prove the full chain works without that shortcut. + +Developer: + Manish Kumar """ from __future__ import annotations @@ -125,13 +128,21 @@ def ctx(monkeypatch, tmp_path): # =========================================================================== class TestExecutePermissionAllowed: + """Valid workflow.execute delegated identities can submit and validate + runs, with existing run-processing business logic unaffected by the + new auth layer.""" + def test_valid_execute_identity_can_submit_run(self, client, monkeypatch): + """A delegated identity holding workflow.execute can POST /runs + and receive a run_id.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post("/runs", json=VALID_BODY, headers=_bearer()) assert resp.status_code == 200 assert "run_id" in resp.json() def test_valid_execute_identity_can_validate(self, client, monkeypatch): + """A delegated identity holding workflow.execute can POST + /validate and get a successful validation result.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post("/validate", json=VALID_BODY, headers=_bearer()) assert resp.status_code == 200 @@ -155,6 +166,9 @@ def test_authenticated_run_still_reaches_completed(self, ctx, monkeypatch): assert store.get(run_id).state == "COMPLETED" def test_validation_failure_business_logic_unaffected_by_auth(self, client, monkeypatch): + """An authenticated request with invalid tool inputs still fails + with the pre-existing VALIDATION_FAILED business error, not an + auth error.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post( "/runs", @@ -170,6 +184,9 @@ def test_validation_failure_business_logic_unaffected_by_auth(self, client, monk # =========================================================================== class TestReadPermissionAllowed: + """Valid runs.read delegated identities can read run status, logs, + and results for existing runs.""" + def _seed(self, store, run_id="known-run", state="COMPLETED", results=None, organization_id="org-7"): store.create(RunRecord( run_id=run_id, tool_id="enrichr_pathway", state=state, @@ -180,6 +197,8 @@ def _seed(self, store, run_id="known-run", state="COMPLETED", results=None, orga )) def test_valid_read_identity_can_get_status(self, ctx, monkeypatch): + """A delegated identity holding runs.read can GET a known run's + status.""" client, store = ctx self._seed(store) _mock_iam(monkeypatch, return_value=READ_IDENTITY) @@ -188,6 +207,8 @@ def test_valid_read_identity_can_get_status(self, ctx, monkeypatch): assert resp.json()["run_id"] == "known-run" def test_valid_read_identity_can_get_logs(self, ctx, monkeypatch): + """A delegated identity holding runs.read can GET a known run's + logs.""" client, store = ctx self._seed(store) _mock_iam(monkeypatch, return_value=READ_IDENTITY) @@ -196,6 +217,8 @@ def test_valid_read_identity_can_get_logs(self, ctx, monkeypatch): assert "line-1" in resp.json()["logs"] def test_valid_read_identity_can_get_results(self, ctx, monkeypatch): + """A delegated identity holding runs.read can GET a known run's + results.""" client, store = ctx self._seed(store, results={"ok": True, "results": {"hits": 3}}) _mock_iam(monkeypatch, return_value=READ_IDENTITY) @@ -209,17 +232,27 @@ def test_valid_read_identity_can_get_results(self, ctx, monkeypatch): # =========================================================================== class TestAuthenticationDenied: + """Every way a request can fail to present a valid delegated-execution + credential -- a missing/malformed Authorization header, or a token + Auth's introspection rejects as not-delegated, expired, revoked, + wrong-audience, wrong-type, malformed, or valid:false -- is denied + with 401, with no silent fallback.""" + def test_missing_bearer_denied(self, client, monkeypatch): + """No Authorization header at all is denied with 401, even though + the mocked IAM client would authorize the identity if reached.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) # would succeed if reached resp = client.post("/runs", json=VALID_BODY) assert resp.status_code == 401 def test_malformed_bearer_denied(self, client, monkeypatch): + """A non-Bearer Authorization scheme (Basic) is denied with 401.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post("/runs", json=VALID_BODY, headers={"Authorization": "Basic abc123"}) assert resp.status_code == 401 def test_bearer_with_no_token_denied(self, client, monkeypatch): + """A bare "Bearer" header with no token value is denied with 401.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post("/runs", json=VALID_BODY, headers={"Authorization": "Bearer"}) assert resp.status_code == 401 @@ -233,31 +266,43 @@ def test_ordinary_user_token_denied(self, client, monkeypatch): assert resp.status_code == 401 def test_ordinary_service_credentials_token_denied(self, client, monkeypatch): + """A service-to-service client-credentials JWT is not a + delegated-execution token; Auth's introspection surfaces it as + invalid, denied with 401.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer("client-credentials-jwt")) assert resp.status_code == 401 def test_invalid_delegated_token_denied(self, client, monkeypatch): + """A forged delegated-execution JWT that Auth's introspection + rejects is denied with 401.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer("forged-delegated-jwt")) assert resp.status_code == 401 def test_expired_delegated_token_denied(self, client, monkeypatch): + """An expired delegated-execution token is denied with 401, via + Auth's own exp check surfacing as an invalid identity.""" _mock_iam(monkeypatch, return_value=None) # Auth's own exp check -> valid:false -> None resp = client.post("/runs", json=VALID_BODY, headers=_bearer("expired-delegated-jwt")) assert resp.status_code == 401 def test_revoked_delegated_token_denied(self, client, monkeypatch): + """A revoked delegated-execution token is denied with 401.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer("revoked-delegated-jwt")) assert resp.status_code == 401 def test_wrong_audience_denied(self, client, monkeypatch): + """A token issued for the wrong audience is denied with 401, via + Auth's own aud check surfacing as an invalid identity.""" _mock_iam(monkeypatch, return_value=None) # Auth's own aud check -> valid:false -> None resp = client.post("/runs", json=VALID_BODY, headers=_bearer("wrong-audience-jwt")) assert resp.status_code == 401 def test_wrong_token_type_denied(self, client, monkeypatch): + """A token of the wrong type (not delegated_execution) is denied + with 401.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer("wrong-type-jwt")) assert resp.status_code == 401 @@ -273,11 +318,16 @@ def test_auth_unavailable_denied(self, client, monkeypatch): assert resp.status_code == 401 def test_malformed_introspection_response_denied(self, client, monkeypatch): + """A malformed/unparseable introspection response from Auth is + treated as an invalid identity and denied with 401 -- fails + closed rather than raising.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer()) assert resp.status_code == 401 def test_valid_false_denied(self, client, monkeypatch): + """An introspection response of {"valid": false} is denied with + 401.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer()) assert resp.status_code == 401 @@ -296,12 +346,22 @@ def test_no_anonymous_fallback_after_denial(self, client, monkeypatch): # =========================================================================== class TestPermissionEnforcement: + """An authenticated identity that lacks the specific delegated + permission a route requires is denied with 403, distinct from the + 401 used for authentication failures; permission scopes are checked + per route, not just per identity.""" + def test_execute_identity_lacking_workflow_execute_denied_403(self, client, monkeypatch): + """An authenticated identity whose permission check raises (Auth + denies workflow.execute) is denied with 403 on POST /runs, not + 401.""" _mock_iam(monkeypatch, side_effect=AuthorizationError("missing permission")) resp = client.post("/runs", json=VALID_BODY, headers=_bearer()) assert resp.status_code == 403 def test_read_identity_lacking_runs_read_denied_403(self, ctx, monkeypatch): + """An authenticated identity lacking runs.read is denied with 403 + on GET /runs/{run_id}, even though the run exists.""" client, store = ctx store.create(RunRecord( run_id="r1", tool_id="enrichr_pathway", state="COMPLETED", @@ -332,6 +392,8 @@ async def _validate(token, permission): assert resp.status_code == 403 def test_workflow_execute_cannot_substitute_for_runs_read(self, ctx, monkeypatch): + """An identity holding only workflow.execute cannot read run + status -- GET /runs/{run_id} is denied with 403.""" client, store = ctx store.create(RunRecord( run_id="r2", tool_id="enrichr_pathway", state="COMPLETED", @@ -354,6 +416,9 @@ async def _validate(token, permission): assert resp.status_code == 403 def test_identity_with_both_permissions_can_do_both(self, ctx, monkeypatch): + """An identity holding both workflow.execute and runs.read can + submit a run and read its status, each permission checked + independently for its own route.""" client, store = ctx store.create(RunRecord( run_id="r3", tool_id="enrichr_pathway", state="COMPLETED", @@ -381,6 +446,12 @@ async def _validate(token, permission): # =========================================================================== class TestHeaderForgeryResistance: + """Client-supplied identity/permission headers (X-User-Id, X-Org-Id, + X-Roles, X-Permissions, X-Service-Id) carry no authority -- only the + delegated-execution identity Auth's introspection returns for the + bearer token is ever used, so forged headers can't grant, substitute + for, or alter the authenticated identity.""" + FORGED_HEADERS = { "X-User-Id": "attacker", "X-Org-Id": "attacker-org", @@ -391,6 +462,10 @@ class TestHeaderForgeryResistance: } def test_forged_headers_alongside_valid_credential_do_not_change_outcome(self, client, monkeypatch): + """Forged identity headers sent alongside a genuinely valid bearer + don't change the outcome or leak into the IAM validation call -- + validate_delegated_execution is invoked with exactly + (token, permission), nothing derived from the forged headers.""" mock = _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post( "/runs", json=VALID_BODY, @@ -403,6 +478,9 @@ def test_forged_headers_alongside_valid_credential_do_not_change_outcome(self, c mock.validate_delegated_execution.assert_called_once_with("delegated.jwt.token", "workflow.execute") def test_forged_headers_cannot_grant_authority_without_a_bearer(self, client, monkeypatch): + """Forged headers with no Authorization header at all are still + denied with 401 -- headers alone never substitute for a bearer + credential.""" _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) resp = client.post("/runs", json=VALID_BODY, headers=self.FORGED_HEADERS) assert resp.status_code == 401 @@ -418,6 +496,9 @@ def test_forged_permissions_header_cannot_add_authority_beyond_identity(self, cl assert resp.status_code == 403 def test_forged_service_header_cannot_alter_calling_service(self, client, monkeypatch): + """A forged X-Service-Id header cannot override the + calling_service on the identity actually returned by Auth + introspection.""" mock = _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) client.post( "/runs", json=VALID_BODY, @@ -434,19 +515,30 @@ def test_forged_service_header_cannot_alter_calling_service(self, client, monkey # =========================================================================== class TestCredentialSafety: + """The bearer credential itself is never echoed back to the caller or + persisted anywhere -- not in error response bodies, not in the run + store, not in result metadata -- and no logging call site in the + security module could leak it either.""" + SECRET_TOKEN = "super-secret-delegated-bearer-value" def test_bearer_absent_from_401_response_body(self, client, monkeypatch): + """A denied request's 401 response body never contains the + bearer token that was rejected.""" _mock_iam(monkeypatch, return_value=None) resp = client.post("/runs", json=VALID_BODY, headers=_bearer(self.SECRET_TOKEN)) assert self.SECRET_TOKEN not in resp.text def test_bearer_absent_from_403_response_body(self, client, monkeypatch): + """A permission-denied request's 403 response body never contains + the bearer token that was used.""" _mock_iam(monkeypatch, side_effect=AuthorizationError("missing permission")) resp = client.post("/runs", json=VALID_BODY, headers=_bearer(self.SECRET_TOKEN)) assert self.SECRET_TOKEN not in resp.text def test_bearer_absent_from_run_store(self, ctx, monkeypatch): + """The bearer token used to authenticate a run's creation is + never persisted in that run's stored record.""" client, store = ctx _mock_iam(monkeypatch, return_value=EXECUTE_IDENTITY) run_id = client.post("/runs", json=VALID_BODY, headers=_bearer(self.SECRET_TOKEN)).json()["run_id"] @@ -454,6 +546,8 @@ def test_bearer_absent_from_run_store(self, ctx, monkeypatch): assert self.SECRET_TOKEN not in rec.model_dump_json() def test_bearer_absent_from_result_metadata(self, ctx, monkeypatch): + """The bearer token is never present in a completed run's + /results response, even after the run finishes processing.""" import time client, store = ctx @@ -484,17 +578,28 @@ def test_bearer_absent_from_security_module_source(self): # =========================================================================== class TestPublicRoutesUnaffected: + """/health and /capabilities are intentionally public -- the + delegated-auth layer added by HIPAA-V2-019 requires no credential on + these routes, and forged identity headers sent to them are simply + ignored rather than granting or denying anything.""" + def test_health_requires_no_credential(self, client): + """GET /health succeeds with no Authorization header at all.""" resp = client.get("/health") assert resp.status_code == 200 assert resp.json()["ok"] is True def test_capabilities_requires_no_credential(self, client): + """GET /capabilities succeeds with no Authorization header, + returning the tool engine listing.""" resp = client.get("/capabilities") assert resp.status_code == 200 assert "engines" in resp.json() def test_health_ignores_forged_headers_and_still_public(self, client): + """GET /health still succeeds (ignoring, not rejecting) when + forged identity/permission headers are present, since the route + enforces no auth at all.""" resp = client.get("/health", headers={"X-User-Id": "attacker", "X-Permissions": "admin"}) assert resp.status_code == 200 @@ -504,6 +609,11 @@ def test_health_ignores_forged_headers_and_still_public(self, client): # =========================================================================== class TestNoAnonymousFallbackAcrossRoutes: + """Every protected route -- not just /runs -- denies a fully + unauthenticated request with 401, even when the (unreachable) mocked + identity would otherwise be authorized; there is no route where + authentication is accidentally skipped.""" + @pytest.mark.parametrize("method,path,body", [ ("post", "/validate", VALID_BODY), ("post", "/runs", VALID_BODY), @@ -512,6 +622,10 @@ class TestNoAnonymousFallbackAcrossRoutes: ("get", "/runs/some-run/results", None), ]) def test_route_denies_without_any_credential(self, client, monkeypatch, method, path, body): + """Parametrized across /validate, /runs, and the three + GET /runs/{id}* read routes: each denies with 401 when no + Authorization header is sent, regardless of what the mocked IAM + client would have returned had it been reached.""" _mock_iam(monkeypatch, return_value=BOTH_IDENTITY) # would succeed if reached call = getattr(client, method) resp = call(path, json=body) if body is not None else call(path) @@ -523,7 +637,16 @@ def test_route_denies_without_any_credential(self, client, monkeypatch, method, # =========================================================================== class TestRegisterToolsUnresolved: + """POST /register_tools stays unconditionally disabled (501) under + the new auth layer too -- neither a valid delegated credential nor + the absence of one changes that outcome, since the endpoint is + disabled before any permission check runs.""" + def test_register_tools_denied_even_with_valid_credential(self, client, monkeypatch): + """A request carrying a fully valid, both-permissions delegated + credential still gets 501 from /register_tools -- the disabled + endpoint is reached before any permission check, so no + credential can unlock it.""" _mock_iam(monkeypatch, return_value=BOTH_IDENTITY) resp = client.post( "/register_tools", json={"tools": [{"tool_id": "x"}]}, headers=_bearer(), @@ -531,6 +654,9 @@ def test_register_tools_denied_even_with_valid_credential(self, client, monkeypa assert resp.status_code == 501 def test_register_tools_denied_without_any_credential(self, client): + """A request with no credential at all also gets 501, the same + as an authenticated one -- proving the 501 comes from the + endpoint being disabled, not from an auth failure.""" resp = client.post("/register_tools", json={"tools": [{"tool_id": "x"}]}) assert resp.status_code == 501 @@ -542,7 +668,21 @@ def test_register_tools_denied_without_any_credential(self, client): # =========================================================================== class TestFullChainWithoutShortcut: + """Re-proves the allow and deny paths one layer deeper than the rest + of this file -- only the raw httpx call inside + AsyncIAMClient.validate_delegated_execution is mocked, so the real + client method, its introspection request construction, and its + response parsing all run for real, confirming the shortcut used + elsewhere (mocking validate_delegated_execution directly) doesn't + hide a break in the client itself.""" + def test_real_validate_delegated_execution_through_mocked_http(self, client, monkeypatch): + """With only the raw httpx POST mocked to return a valid + introspection response, the real + AsyncIAMClient.validate_delegated_execution constructs the + correct introspection request (POST to + .../service/delegations/toolserver/introspect) and the run + succeeds end-to-end.""" real_client = security_mod.AsyncIAMClient(base_url="http://fake-auth") mock_response = MagicMock() mock_response.status_code = 200 @@ -567,6 +707,10 @@ def test_real_validate_delegated_execution_through_mocked_http(self, client, mon assert url.endswith("/service/delegations/toolserver/introspect") def test_real_chain_denies_valid_false(self, client, monkeypatch): + """With only the raw httpx POST mocked to return + {"valid": false}, the real client chain still denies the request + with 401 -- the fail-closed behavior holds through the full, + unshortcut path.""" real_client = security_mod.AsyncIAMClient(base_url="http://fake-auth") mock_response = MagicMock() mock_response.status_code = 200 diff --git a/tests/test_toolserver_run_ownership.py b/tests/test_toolserver_run_ownership.py index c641883..888dd81 100644 --- a/tests/test_toolserver_run_ownership.py +++ b/tests/test_toolserver_run_ownership.py @@ -9,6 +9,9 @@ already covered there; this file focuses on the orthogonal condition this task adds: a valid, correctly-permissioned delegated identity from the WRONG organization must still be denied. + +Developer: + Manish Kumar """ from __future__ import annotations @@ -56,6 +59,9 @@ def _make_app_client(monkeypatch, tmp_path): + """Build a real create_app() FastAPI TestClient against an isolated + RunStore directory, with the tool execution itself faked out -- only + the ownership/authorization plumbing under test is real.""" import toolserver.tools as tools_mod def fake_run(inputs, resources, log): @@ -119,6 +125,9 @@ def _submit(client, identity_setter, **headers) -> str: def _seed(store, run_id, *, organization_id, state="COMPLETED", results=None, logs=None): + """Write a RunRecord straight into the store, bypassing the API -- + lets tests plant a run "owned" by an arbitrary (including foreign, + null, or malformed) organization_id to probe read-side enforcement.""" store.create(RunRecord( run_id=run_id, tool_id="enrichr_pathway", state=state, created_epoch=1_700_000_000, updated_epoch=1_700_000_001, @@ -134,13 +143,19 @@ def _seed(store, run_id, *, organization_id, state="COMPLETED", results=None, lo # =========================================================================== class TestOwnershipAssignedAtCreation: + """A newly created run is stamped with the organization_id taken from + the caller's verified delegated identity, never a default or guess.""" + def test_new_run_stores_authenticated_organization(self, ctx, monkeypatch): + """Persist the delegated identity's organization on the created RunRecord.""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) run_id = _submit(client, EXECUTE_A) assert store.get(run_id).organization_id == ORG_A def test_organization_comes_from_delegated_identity_not_a_default(self, ctx, monkeypatch): + """Stamp the run with whatever organization the identity carries, + not a hardcoded/expected value -- rules out a stray constant.""" client, store = ctx identity = DelegatedExecutionIdentity( calling_service="tes-service", initiating_user="user-x", organization="org-unusual-42", @@ -156,7 +171,13 @@ def test_organization_comes_from_delegated_identity_not_a_default(self, ctx, mon # =========================================================================== class TestCreationForgeryResistance: + """A run's owning organization is derived solely from the verified + delegated identity; nothing a caller sends in the request cannot + forge a different owner at creation time.""" + def test_body_organization_id_in_inputs_cannot_override(self, ctx, monkeypatch): + """A forged organization_id smuggled into `inputs` is ignored; + the run is still owned by the caller's real organization.""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) body = { @@ -170,6 +191,8 @@ def test_body_organization_id_in_inputs_cannot_override(self, ctx, monkeypatch): assert store.get(run_id).organization_id == ORG_A def test_metadata_organization_id_in_resources_cannot_override(self, ctx, monkeypatch): + """A forged organization_id/org_id smuggled into `resources` is + ignored; the run is still owned by the caller's real organization.""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) body = { @@ -183,6 +206,8 @@ def test_metadata_organization_id_in_resources_cannot_override(self, ctx, monkey assert store.get(run_id).organization_id == ORG_A def test_forged_org_headers_cannot_override_at_creation(self, ctx, monkeypatch): + """Forged X-Org-Id/X-Organization-Id request headers are ignored; + the run is still owned by the caller's real organization.""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) run_id = _submit( @@ -197,7 +222,13 @@ def test_forged_org_headers_cannot_override_at_creation(self, ctx, monkeypatch): # =========================================================================== class TestSerializationPreservesOwnership: + """organization_id survives RunRecord (de)serialization, both a pure + in-memory JSON round-trip and a real read from a second store instance + backed by the same on-disk directory.""" + def test_serialization_round_trip_preserves_organization(self): + """A RunRecord's organization_id is unchanged after a + model_dump_json -> model_validate_json round trip.""" rec = RunRecord( run_id="ser-1", tool_id="t", state="QUEUED", created_epoch=1, updated_epoch=1, organization_id=ORG_A, @@ -224,7 +255,11 @@ def test_deserialization_from_disk_preserves_organization(self, ctx, monkeypatch # =========================================================================== class TestSameOrgReadAllowed: + """A delegated identity with runs.read for the run's owning + organization can read that run's status, logs, and results.""" + def test_status_same_org_allowed(self, ctx, monkeypatch): + """GET /runs/{id} returns the run to a same-org reader.""" client, store = ctx _seed(store, "run-1", organization_id=ORG_A, state="RUNNING") _mock_iam_fixed(monkeypatch, READ_A) @@ -233,6 +268,8 @@ def test_status_same_org_allowed(self, ctx, monkeypatch): assert resp.json()["run_id"] == "run-1" def test_logs_same_org_allowed(self, ctx, monkeypatch): + """GET /runs/{id}/logs returns the real log content to a + same-org reader.""" client, store = ctx _seed(store, "run-2", organization_id=ORG_A, logs=["hello-from-org-a"]) _mock_iam_fixed(monkeypatch, READ_A) @@ -241,6 +278,8 @@ def test_logs_same_org_allowed(self, ctx, monkeypatch): assert "hello-from-org-a" in resp.json()["logs"] def test_results_same_org_allowed(self, ctx, monkeypatch): + """GET /runs/{id}/results returns the real result payload to a + same-org reader.""" client, store = ctx _seed(store, "run-3", organization_id=ORG_A, results={"ok": True, "results": {"hits": 7}}) _mock_iam_fixed(monkeypatch, READ_A) @@ -266,7 +305,14 @@ def test_same_org_different_user_allowed(self, ctx, monkeypatch): # =========================================================================== class TestCrossTenantDenied: + """A valid, correctly-permissioned delegated identity from the WRONG + organization is denied read access to another organization's run -- + the central requirement this task adds -- and the denial reveals + neither the run's real state nor any of its log/result content.""" + def test_status_wrong_org_denied(self, ctx, monkeypatch): + """A cross-tenant status read gets ToolServer's own "unknown run" + shape (200 + FAILED/unknown run), not the run's real state.""" client, store = ctx _seed(store, "org-a-run", organization_id=ORG_A) _mock_iam_fixed(monkeypatch, READ_B) @@ -275,6 +321,8 @@ def test_status_wrong_org_denied(self, ctx, monkeypatch): assert resp.json() == {"state": "FAILED", "message": "unknown run"} def test_logs_wrong_org_denied(self, ctx, monkeypatch): + """A cross-tenant logs read never leaks the real log lines -- + it gets the "unknown run" logs message instead.""" client, store = ctx _seed(store, "org-a-run", organization_id=ORG_A, logs=["ORG-A-SECRET-LOG-LINE"]) _mock_iam_fixed(monkeypatch, READ_B) @@ -284,6 +332,8 @@ def test_logs_wrong_org_denied(self, ctx, monkeypatch): assert resp.json()["logs"] == "[org-a-run] unknown run" def test_results_wrong_org_denied(self, ctx, monkeypatch): + """A cross-tenant results read never leaks the real result + payload -- it gets a NOT_FOUND error instead.""" client, store = ctx _seed( store, "org-a-run", organization_id=ORG_A, @@ -312,7 +362,14 @@ def test_exact_valid_foreign_run_id_still_denied(self, ctx, monkeypatch): # =========================================================================== class TestPermissionAndOwnershipBothRequired: + """runs.read permission and same-organization ownership are + independent, both-required gates: missing permission denies with 403 + regardless of org match, and permission alone never substitutes for + ownership (a wrong-org reader with valid runs.read still gets the + ownership-layer "unknown run" denial, not a 403).""" + def test_correct_org_no_runs_read_denied(self, ctx, monkeypatch): + """Same org but missing runs.read is a 403, not an ownership denial.""" client, store = ctx _seed(store, "run-x", organization_id=ORG_A) _mock_iam_fixed(monkeypatch, EXECUTE_A_NO_PERMS) # right org, no runs.read @@ -320,6 +377,8 @@ def test_correct_org_no_runs_read_denied(self, ctx, monkeypatch): assert resp.status_code == 403 def test_wrong_org_no_runs_read_denied(self, ctx, monkeypatch): + """Wrong org and missing runs.read together still surface as the + permission-layer 403.""" client, store = ctx _seed(store, "run-x", organization_id=ORG_A) _mock_iam_fixed(monkeypatch, READ_B_NO_PERMS) # wrong org AND no runs.read @@ -327,6 +386,8 @@ def test_wrong_org_no_runs_read_denied(self, ctx, monkeypatch): assert resp.status_code == 403 def test_wrong_org_with_valid_runs_read_denied(self, ctx, monkeypatch): + """Holding runs.read does not let a wrong-org identity read the + run -- ownership enforcement still denies it as unknown.""" client, store = ctx _seed(store, "run-x", organization_id=ORG_A) _mock_iam_fixed(monkeypatch, READ_B) # valid permission, wrong org @@ -335,6 +396,7 @@ def test_wrong_org_with_valid_runs_read_denied(self, ctx, monkeypatch): assert resp.json()["message"] == "unknown run" def test_correct_org_with_valid_runs_read_allowed(self, ctx, monkeypatch): + """Same org plus valid runs.read is the only combination that's allowed.""" client, store = ctx _seed(store, "run-x", organization_id=ORG_A) _mock_iam_fixed(monkeypatch, READ_A) @@ -348,7 +410,14 @@ def test_correct_org_with_valid_runs_read_allowed(self, ctx, monkeypatch): # =========================================================================== class TestFailClosedOnMissingOwnership: + """A run with no usable organization_id -- absent (pre-V2-001 legacy + record), None, empty string, or malformed -- is denied to every + reader, never treated as ownerless-therefore-public or matched by + default/wildcard logic.""" + def test_ownerless_legacy_status_denied(self, ctx, monkeypatch): + """A RunRecord written before organization_id existed (field + entirely absent) is denied on status read, not treated as public.""" client, store = ctx store.create(RunRecord( run_id="legacy-1", tool_id="enrichr_pathway", state="COMPLETED", @@ -359,6 +428,8 @@ def test_ownerless_legacy_status_denied(self, ctx, monkeypatch): assert resp.json() == {"state": "FAILED", "message": "unknown run"} def test_ownerless_legacy_logs_denied(self, ctx, monkeypatch): + """A legacy ownerless record's logs are denied, and the real log + content never leaks into the denial response.""" client, store = ctx store.create(RunRecord( run_id="legacy-2", tool_id="enrichr_pathway", state="COMPLETED", @@ -370,6 +441,8 @@ def test_ownerless_legacy_logs_denied(self, ctx, monkeypatch): assert resp.json()["logs"] == "[legacy-2] unknown run" def test_ownerless_legacy_results_denied(self, ctx, monkeypatch): + """A legacy ownerless record's results are denied, and the real + result payload never leaks into the denial response.""" client, store = ctx store.create(RunRecord( run_id="legacy-3", tool_id="enrichr_pathway", state="COMPLETED", @@ -381,6 +454,7 @@ def test_ownerless_legacy_results_denied(self, ctx, monkeypatch): assert resp.json()["error"]["code"] == "NOT_FOUND" def test_null_owner_denied(self, ctx, monkeypatch): + """organization_id=None is denied, not matched by any reader.""" client, store = ctx _seed(store, "null-owner", organization_id=None) _mock_iam_fixed(monkeypatch, READ_A) @@ -388,6 +462,7 @@ def test_null_owner_denied(self, ctx, monkeypatch): assert resp.json()["message"] == "unknown run" def test_empty_owner_denied(self, ctx, monkeypatch): + """organization_id="" is denied, not treated as a wildcard match.""" client, store = ctx _seed(store, "empty-owner", organization_id="") _mock_iam_fixed(monkeypatch, READ_A) @@ -423,7 +498,11 @@ def test_ownerless_run_never_becomes_readable_via_reader_identity(self, ctx, mon # =========================================================================== class TestOwnershipImmutability: + """organization_id, once stamped at run creation, is never altered by + later state/result updates through the run's execution lifecycle.""" + def test_status_update_does_not_alter_owner(self, ctx, monkeypatch): + """Updating a run's state field leaves organization_id unchanged.""" client, store = ctx _seed(store, "life-1", organization_id=ORG_A, state="QUEUED") rec = store.get("life-1") @@ -433,6 +512,7 @@ def test_status_update_does_not_alter_owner(self, ctx, monkeypatch): assert store.get("life-1").organization_id == ORG_A def test_result_update_does_not_alter_owner(self, ctx, monkeypatch): + """Updating a run's results field leaves organization_id unchanged.""" client, store = ctx _seed(store, "life-2", organization_id=ORG_A, state="RUNNING", results=None) rec = store.get("life-2") @@ -442,6 +522,10 @@ def test_result_update_does_not_alter_owner(self, ctx, monkeypatch): assert store.get("life-2").organization_id == ORG_A def test_full_execution_lifecycle_preserves_owner(self, ctx, monkeypatch): + """A run driven end-to-end through real execution (submit -> + poll to COMPLETED) keeps its original owner, and ownership + enforcement still applies to it afterward: the owning org can + read it, a different org still gets "unknown run".""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) run_id = _submit(client, EXECUTE_A) @@ -468,7 +552,14 @@ def test_full_execution_lifecycle_preserves_owner(self, ctx, monkeypatch): # =========================================================================== class TestOwnershipFieldSafety: + """The persisted RunRecord keeps organization_id distinct from other + identity fields: the raw bearer credential is never stored, and + calling_service/initiating_user are never mistaken for the + organization owner even when they're set to unusual values.""" + def test_bearer_not_persisted_alongside_owner(self, ctx, monkeypatch): + """The raw bearer token used to authenticate a run's creation + never appears in that run's persisted JSON record.""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) secret_token = "super-secret-bearer-value-xyz" @@ -478,6 +569,9 @@ def test_bearer_not_persisted_alongside_owner(self, ctx, monkeypatch): assert secret_token not in rec.model_dump_json() def test_calling_service_not_substituted_for_organization(self, ctx, monkeypatch): + """organization_id is stamped from identity.organization, never + from identity.calling_service, even when calling_service is set + to an unrelated-looking value.""" identity = DelegatedExecutionIdentity( calling_service="a-completely-different-value", initiating_user="user-a1", organization=ORG_A, delegated_permissions=frozenset({"workflow.execute"}), @@ -491,6 +585,9 @@ def test_calling_service_not_substituted_for_organization(self, ctx, monkeypatch assert rec.organization_id != identity.calling_service def test_initiating_user_not_substituted_for_organization(self, ctx, monkeypatch): + """organization_id is stamped from identity.organization, never + from identity.initiating_user, even when initiating_user is set + to an unrelated-looking value.""" identity = DelegatedExecutionIdentity( calling_service="tes-service", initiating_user="a-completely-different-value", organization=ORG_A, delegated_permissions=frozenset({"workflow.execute"}), @@ -509,16 +606,25 @@ def test_initiating_user_not_substituted_for_organization(self, ctx, monkeypatch # =========================================================================== class TestUnaffectedSurfaces: + """Routes with no per-run ownership semantics (health, capabilities, + validate, the already-disabled register_tools) keep their pre-V2-001 + behavior unchanged by the ownership feature.""" + def test_health_unaffected(self, client): + """/health remains a public, unauthenticated 200 OK.""" resp = client.get("/health") assert resp.status_code == 200 assert resp.json()["ok"] is True def test_capabilities_unaffected(self, client): + """/capabilities remains reachable and unaffected by ownership.""" resp = client.get("/capabilities") assert resp.status_code == 200 def test_validate_protected_but_creates_no_run_record(self, ctx, monkeypatch): + """/validate still requires a valid delegated credential but, + since it's not a persisted tenant resource, creates no RunRecord + and therefore has no ownership semantics of its own.""" client, store = ctx _mock_iam_fixed(monkeypatch, EXECUTE_A) resp = client.post("/validate", json=VALID_BODY, headers=_bearer()) @@ -529,10 +635,14 @@ def test_validate_protected_but_creates_no_run_record(self, ctx, monkeypatch): assert len(list(store.root.glob("*.json"))) == 0 def test_validate_still_denied_without_credential(self, client): + """/validate still 401s with no bearer credential at all.""" resp = client.post("/validate", json=VALID_BODY) assert resp.status_code == 401 def test_register_tools_remains_fail_closed(self, client, monkeypatch): + """/register_tools remains unconditionally disabled (501) even + for an otherwise validly-authenticated caller -- unaffected by + the ownership feature.""" _mock_iam_fixed(monkeypatch, EXECUTE_A) resp = client.post("/register_tools", json={"tools": [{"tool_id": "x"}]}, headers=_bearer()) assert resp.status_code == 501 @@ -544,6 +654,11 @@ def test_register_tools_remains_fail_closed(self, client, monkeypatch): # =========================================================================== class TestNoOwnershipBypass: + """No route (status, logs, results) can surface a foreign-org run's + real content by any path other than the ownership-checked RunRecord + lookup -- each seeds unmistakable sentinel content and asserts it + never reaches a denied caller.""" + def test_logs_route_never_reads_store_directly_without_authorization(self, ctx, monkeypatch): """Regression guard for the exact bypass class this task calls out: a route must not be able to construct/return log content @@ -557,6 +672,8 @@ def test_logs_route_never_reads_store_directly_without_authorization(self, ctx, assert "UNMISTAKABLE-SENTINEL-VALUE-42" not in resp.text def test_results_route_never_reads_store_directly_without_authorization(self, ctx, monkeypatch): + """A foreign-org run's real result payload never appears in the + results route's denial response, even as a raw substring.""" client, store = ctx _seed(store, "bypass-check-2", organization_id=ORG_A, results={"ok": True, "results": {"marker": "UNMISTAKABLE-SENTINEL-VALUE-99"}}) @@ -565,6 +682,8 @@ def test_results_route_never_reads_store_directly_without_authorization(self, ct assert "UNMISTAKABLE-SENTINEL-VALUE-99" not in resp.text def test_status_route_never_reveals_state_of_foreign_run(self, ctx, monkeypatch): + """A foreign-org run's real state (e.g. RUNNING) never leaks + through the status route's denial response.""" client, store = ctx _seed(store, "bypass-check-3", organization_id=ORG_A, state="RUNNING") _mock_iam_fixed(monkeypatch, READ_B)