From acf8f69dccf148e78bbed4f926459fc932674e70 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 16:55:36 +0000 Subject: [PATCH 1/2] Fix test result propagation to radiator and Slack Several test results were not making it to the reporting services intact: - Performance tests were created in the Information Radiator but never finished, so they showed up as perpetually incomplete. run_tests now finishes each performance test with a terminal status (FAILED if the query can't be generated or the run raises, PASSED otherwise). - Performance failures were overwritten per host in the ResultCollector, so when a suite exercised more than one target only the last target's failures reached Slack. Failures are now accumulated by error key, summing occurrences across hosts. - Agents that returned no response for an asset were written to the CSV as SKIPPED but omitted from the per-agent JSON stats, so the two Slack artifacts disagreed and undercounted. Skipped agents are now counted in the JSON stats too. - ARS timeouts (status code 598) were compared against the string "598" and so were never labeled "Timed out"; compare as a string of the code. Adds regression tests for each fix. --- test_harness/result_collector.py | 19 ++- test_harness/run.py | 43 ++++-- tests/test_reporting.py | 234 +++++++++++++++++++++++++++++++ 3 files changed, 282 insertions(+), 14 deletions(-) create mode 100644 tests/test_reporting.py diff --git a/test_harness/result_collector.py b/test_harness/result_collector.py index cff10d2..d0aa5be 100644 --- a/test_harness/result_collector.py +++ b/test_harness/result_collector.py @@ -261,6 +261,12 @@ def collect_acceptance_result( self.acceptance_stats[agent][query_type][agent_result.status.value] += 1 agent_statuses.append(agent_result.status.value) else: + # Agent produced no response for this asset. Record it as + # SKIPPED in the per-agent stats too, so the JSON summary + # uploaded to Slack stays consistent with the CSV (which + # already reports SKIPPED here) and every asset is accounted + # for in each agent's totals. + self.acceptance_stats[agent][query_type][AgentStatus.SKIPPED.value] += 1 agent_statuses.append(AgentStatus.SKIPPED.value) # add result to csv @@ -305,7 +311,18 @@ def collect_performance_result( "history": results.get("stats_history") or [], "summary_html": results.get("summary_html"), } - self.performance_report["failures"] = results.get("failures") or {} + # Accumulate failures across every performance target/asset. This used + # to be a plain assignment, which meant only the last target's failures + # ever reached Slack when a suite exercised more than one host. Merge by + # error key, summing occurrences so the summary reflects the whole run. + for failure_key, failure in (results.get("failures") or {}).items(): + existing = self.performance_report["failures"].get(failure_key) + if existing: + existing["occurrences"] = existing.get("occurrences", 0) + failure.get( + "occurrences", 0 + ) + else: + self.performance_report["failures"][failure_key] = dict(failure) stats_id = f"{host_url}_case_{test.id}_asset_{asset.id}" self.performance_stats[stats_id] = { diff --git a/test_harness/run.py b/test_harness/run.py index 58e2d3e..6c7e5cb 100644 --- a/test_harness/run.py +++ b/test_harness/run.py @@ -116,7 +116,7 @@ def run_tests( try: if response["status_code"] > 299: agent_report.status = AgentStatus.FAILED - if response["status_code"] == "598": + if str(response["status_code"]) == "598": agent_report.message = "Timed out" else: agent_report.message = ( @@ -236,18 +236,35 @@ def run_tests( test_id, message, ) - host = query_runner.registry[env_map[test.test_env]][ - test.components[0] - ][0]["url"] - results = run_performance_test(test, test_query, host) - - collector.collect_performance_result( - test, - asset, - f"{reporter.base_path}/test-runs/{reporter.test_run_id}/tests/{test_id}", - host, - results, - ) + # Give the performance test a terminal status in the + # Information Radiator. Without this the test is created + # but never finished, so it shows up as perpetually + # incomplete in the dashboard. + status = AgentStatus.PASSED + if test_query is None: + logger.error( + f"Unable to generate performance query for asset: {asset.id}" + ) + status = AgentStatus.FAILED + else: + host = query_runner.registry[env_map[test.test_env]][ + test.components[0] + ][0]["url"] + try: + results = run_performance_test(test, test_query, host) + collector.collect_performance_result( + test, + asset, + f"{reporter.base_path}/test-runs/{reporter.test_run_id}/tests/{test_id}", + host, + results, + ) + except Exception as e: + logger.error( + f"Failed to run performance test for {test.id}: {e}" + ) + status = AgentStatus.FAILED + reporter.finish_test(test_id, status.value) # try: # test_inputs = [ # assets.id, diff --git a/tests/test_reporting.py b/tests/test_reporting.py new file mode 100644 index 0000000..dd1742b --- /dev/null +++ b/tests/test_reporting.py @@ -0,0 +1,234 @@ +"""Regression tests for test-result propagation to reporting services. + +These cover bugs where results were dropped or mangled on their way to the +Information Radiator (Reporter) and/or Slack (via the ResultCollector output): + +* Performance tests were created in the radiator but never finished. +* Performance failures were overwritten per host instead of accumulated. +* Agents that returned no response were written to the CSV but omitted from + the per-agent JSON stats. +""" + +from translator_testing_model.datamodel.pydanticmodel import ( + ComponentEnum, + PerformanceTestCase, + TestEnvEnum, + TestObjectiveEnum, +) + +from test_harness.result_collector import ResultCollector +from test_harness.run import run_tests +from test_harness.utils import AgentReport, AgentStatus, TestReport + +from .helpers.logger import setup_logger +from .helpers.mocks import MockReporter, MockResultCollector, MockQueryRunner + +logger = setup_logger() + + +class _Asset: + name = "asset-name" + id = "asset-1" + expected_output = "TopAnswer" + + +class _Case: + id = "case-1" + + +def test_skipped_agents_are_counted_in_stats(): + """An agent with no response should be recorded as SKIPPED in the JSON + stats, not just the CSV, so the two Slack artifacts agree.""" + collector = ResultCollector("dev", logger) + # Only "ars" responded; the shepherd agents should be counted SKIPPED. + report = TestReport( + pks={}, + result={ + "ars": AgentReport( + status=AgentStatus.PASSED, message=None, actual_output=None + ) + }, + test_details=None, + ) + collector.collect_acceptance_result(_Case(), _Asset(), report, "pk", "http://ir/1") + + for agent in collector.agents: + total = sum(collector.acceptance_stats[agent]["TopAnswer"].values()) + assert total == 1, f"{agent} should have exactly one recorded result" + assert collector.acceptance_stats["ars"]["TopAnswer"]["PASSED"] == 1 + for agent in collector.agents: + if agent == "ars": + continue + assert collector.acceptance_stats[agent]["TopAnswer"]["SKIPPED"] == 1 + + # CSV and JSON should agree: the CSV row lists PASSED then three SKIPPED. + csv_row = collector.acceptance_csv.strip().splitlines()[-1] + assert csv_row.count("SKIPPED") == 3 + assert "PASSED" in csv_row + + +def _perf_results(target, failures): + return { + "stats": [], + "failures": failures, + "test_run_time": 10, + "spawn_rate": 1, + "target": target, + "query_response_sizes": {}, + "stats_history": [], + "summary_html": None, + } + + +def test_performance_failures_accumulate_across_hosts(): + """Failures from every performance target must survive to the summary.""" + collector = ResultCollector("prod", logger) + collector.collect_performance_result( + _Case(), + _Asset(), + "http://ir/1", + "hostA", + _perf_results( + "ars", + { + "k1": { + "method": "POST", + "name": "submit", + "error": "x", + "occurrences": 3, + } + }, + ), + ) + collector.collect_performance_result( + _Case(), + _Asset(), + "http://ir/2", + "hostB", + _perf_results( + "ars", + { + "k1": { + "method": "POST", + "name": "submit", + "error": "x", + "occurrences": 2, + }, + "k2": {"method": "GET", "name": "poll", "error": "y", "occurrences": 5}, + }, + ), + ) + failures = collector.performance_report["failures"] + assert set(failures) == {"k1", "k2"} + # k1 seen on both hosts -> occurrences summed. + assert failures["k1"]["occurrences"] == 5 + assert failures["k2"]["occurrences"] == 5 + total = sum(f.get("occurrences", 0) for f in failures.values()) + assert total == 10 + + +class _RecordingReporter(MockReporter): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.finished = [] + + def finish_test(self, test_id, result): + self.finished.append((test_id, result)) + return result + + +def _performance_test_case(): + return PerformanceTestCase( + id="perf-1", + name="ExamplePerformanceTest", + description="perf", + tags=[], + test_runner_settings=["inferred"], + test_run_time=10, + spawn_rate=0.1, + query_type=None, + test_assets=[ + { + "id": "Asset_1", + "name": "perf asset", + "description": "perf asset", + "tags": [], + "test_runner_settings": ["inferred"], + "input_id": "MONDO:0011426", + "input_name": "Aceruloplasminemia", + "input_category": "biolink:Disease", + "predicate_id": "biolink:treats", + "predicate_name": "treats", + "output_id": "PUBCHEM.COMPOUND:23925", + "output_name": "Iron", + "output_category": "biolink:ChemicalEntity", + "association": None, + "qualifiers": [ + {"parameter": "biolink_object_aspect_qualifier", "value": ""}, + {"parameter": "biolink_object_direction_qualifier", "value": ""}, + ], + "expected_output": "NeverShow", + } + ], + preconditions=[], + trapi_template=None, + test_case_objective=TestObjectiveEnum.QuantitativeTest, + test_case_source=None, + test_case_predicate_name="treats", + test_case_predicate_id="biolink:treats", + test_case_input_id="MONDO:0011426", + qualifiers=[], + input_category="biolink:Disease", + output_category=None, + components=[ComponentEnum.ars], + test_env=TestEnvEnum.ci, + ) + + +def test_performance_test_is_finished_in_radiator(mocker): + """A performance test must reach a terminal status in the radiator.""" + mocker.patch( + "test_harness.run.QueryRunner", + return_value=MockQueryRunner(logger), + ) + mocker.patch( + "test_harness.run.run_performance_test", + return_value=_perf_results("ars", {}), + ) + + reporter = _RecordingReporter(base_url="http://ir") + run_tests( + tests={"TestCase_1": _performance_test_case()}, + reporter=reporter, + collector=MockResultCollector("ci", logger), + logger=logger, + args={"suite": "perf", "trapi_version": "1.6.0"}, + ) + + assert reporter.finished, "performance test was never finished in the radiator" + assert reporter.finished[0][1] == AgentStatus.PASSED.value + + +def test_performance_test_finished_failed_on_error(mocker): + """If the performance run raises, the radiator still gets a terminal + FAILED status instead of a perpetually-unfinished test.""" + mocker.patch( + "test_harness.run.QueryRunner", + return_value=MockQueryRunner(logger), + ) + mocker.patch( + "test_harness.run.run_performance_test", + side_effect=RuntimeError("boom"), + ) + + reporter = _RecordingReporter(base_url="http://ir") + run_tests( + tests={"TestCase_1": _performance_test_case()}, + reporter=reporter, + collector=MockResultCollector("ci", logger), + logger=logger, + args={"suite": "perf", "trapi_version": "1.6.0"}, + ) + + assert reporter.finished + assert reporter.finished[0][1] == AgentStatus.FAILED.value From 0e8d327720c9e18d34697217174ea4d5fa0a3ab2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 17:06:44 +0000 Subject: [PATCH 2/2] Mark skipped test-case assets as SKIPPED across all agents When an acceptance test case is skipped (its query never really ran), its assets were not being reported as SKIPPED. Instead each agent kept its incidental status - FAILED for ARAs that errored, NO_RESULTS for an ARS that returned an empty message - which inflated FAILED/NO_RESULTS counts and left 0 SKIPPED for the ARAs, disagreeing with the skipped test-level status shown in the Information Radiator. Two gaps caused this: - When the test-level status resolves to SKIPPED (no ARS result), the per-agent results were still recorded from the report. collect_acceptance_ result now takes force_skipped so every agent is recorded SKIPPED, and the radiator labels are uploaded as SKIPPED for all agents to match (previously labels were not uploaded at all for skipped tests, leaving the per-agent columns stale/blank). - When an asset had no query response at all (eg query generation failed), it was finished as SKIPPED in the radiator but never passed to the collector, so it vanished from the CSV and per-agent JSON stats. It is now recorded as SKIPPED for every agent. Adds regression tests covering the forced-skip stats/labels and an end-to-end skipped test case. --- test_harness/result_collector.py | 11 +++- test_harness/run.py | 52 ++++++++++++++--- tests/helpers/mocks.py | 5 +- tests/test_reporting.py | 96 ++++++++++++++++++++++++++++++++ 4 files changed, 154 insertions(+), 10 deletions(-) diff --git a/test_harness/result_collector.py b/test_harness/result_collector.py index d0aa5be..2c0fa8d 100644 --- a/test_harness/result_collector.py +++ b/test_harness/result_collector.py @@ -249,14 +249,21 @@ def collect_acceptance_result( report: TestReport, parent_pk: Union[str, None], url: str, + force_skipped: bool = False, ): - """Add a single report to the total output.""" + """Add a single report to the total output. + + ``force_skipped`` records every agent as SKIPPED regardless of what is + in ``report``. It is used when the test as a whole was skipped (eg the + query never ran) so the per-agent stats/CSV agree with the skipped + test-level status instead of reporting incidental per-ARA errors. + """ self.has_acceptance_results = True # add result to stats agent_statuses = [] for agent in self.agents: query_type = asset.expected_output - if agent in report.result: + if not force_skipped and agent in report.result: agent_result = report.result[agent] self.acceptance_stats[agent][query_type][agent_result.status.value] += 1 agent_statuses.append(agent_result.status.value) diff --git a/test_harness/run.py b/test_harness/run.py index 6c7e5cb..064489b 100644 --- a/test_harness/run.py +++ b/test_harness/run.py @@ -179,23 +179,39 @@ def run_tests( agent_report.status = AgentStatus.FAILED agent_report.message = "Test Error" - # grab only ars result if it exists, otherwise default to failed + # The overall test status is driven by ARS. If ARS didn't + # produce a result, the whole test is considered skipped. if "ars" not in report.result: status = AgentStatus.SKIPPED else: status = report.result["ars"].status + # When the test is skipped, every agent is skipped too: the + # query never really ran, so the incidental per-ARA + # error/no-result statuses would be misleading. Force them + # all to SKIPPED so the radiator labels, CSV, and JSON stats + # stay consistent with the skipped test-level status. + force_skipped = status == AgentStatus.SKIPPED + collector.collect_acceptance_result( test, asset, report, test_query["pks"].get("parent_pk"), f"{reporter.base_path}/test-runs/{reporter.test_run_id}/tests/{test_id}", + force_skipped=force_skipped, ) - if status != AgentStatus.SKIPPED: - # only upload ara labels if the test ran successfully - try: + try: + if force_skipped: + labels = [ + { + "key": ara, + "value": AgentStatus.SKIPPED.value, + } + for ara in collector.agents + ] + else: labels = [ { "key": ara, @@ -204,13 +220,35 @@ def run_tests( for ara in collector.agents if ara in report.result ] - reporter.upload_labels(test_id, labels) - except Exception as e: - logger.warning(f"[{test.id}] failed to upload labels: {e}") + reporter.upload_labels(test_id, labels) + except Exception as e: + logger.warning(f"[{test.id}] failed to upload labels: {e}") logger.info(f"Full report: {json.dumps(asdict(report), indent=4)}") reporter.upload_log(test_id, json.dumps(asdict(report), indent=4)) else: + # No query response for this asset (eg query generation + # failed). Record it as skipped across every agent so it + # still appears in the per-agent stats, CSV, and radiator + # labels as SKIPPED instead of being dropped entirely. status = AgentStatus.SKIPPED + collector.collect_acceptance_result( + test, + asset, + TestReport(pks={}, result={}, test_details=None), + None, + f"{reporter.base_path}/test-runs/{reporter.test_run_id}/tests/{test_id}", + force_skipped=True, + ) + try: + reporter.upload_labels( + test_id, + [ + {"key": ara, "value": AgentStatus.SKIPPED.value} + for ara in collector.agents + ], + ) + except Exception as e: + logger.warning(f"[{test.id}] failed to upload labels: {e}") reporter.finish_test(test_id, status.value) collector.acceptance_report[status.value] += 1 diff --git a/tests/helpers/mocks.py b/tests/helpers/mocks.py index 920b557..2d7ba95 100644 --- a/tests/helpers/mocks.py +++ b/tests/helpers/mocks.py @@ -85,8 +85,11 @@ def collect_acceptance_result( report: dict, parent_pk: str | None, url: str, + force_skipped: bool = False, ): - return super().collect_acceptance_result(test, asset, report, parent_pk, url) + return super().collect_acceptance_result( + test, asset, report, parent_pk, url, force_skipped=force_skipped + ) def collect_performance_result( self, diff --git a/tests/test_reporting.py b/tests/test_reporting.py index dd1742b..aa19a94 100644 --- a/tests/test_reporting.py +++ b/tests/test_reporting.py @@ -3,6 +3,9 @@ These cover bugs where results were dropped or mangled on their way to the Information Radiator (Reporter) and/or Slack (via the ResultCollector output): +* A skipped test case left its assets marked FAILED (for ARAs) / NO_RESULTS + (for ARS) instead of SKIPPED, and assets that never got a query were dropped + from the per-agent stats entirely. * Performance tests were created in the radiator but never finished. * Performance failures were overwritten per host instead of accumulated. * Agents that returned no response were written to the CSV but omitted from @@ -20,6 +23,7 @@ from test_harness.run import run_tests from test_harness.utils import AgentReport, AgentStatus, TestReport +from .helpers.example_tests import example_test_cases from .helpers.logger import setup_logger from .helpers.mocks import MockReporter, MockResultCollector, MockQueryRunner @@ -131,11 +135,46 @@ class _RecordingReporter(MockReporter): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.finished = [] + self.labels = [] def finish_test(self, test_id, result): self.finished.append((test_id, result)) return result + def upload_labels(self, test_id, labels): + self.labels.append(labels) + + +def test_force_skipped_records_all_agents_skipped(): + """A skipped test must mark every agent SKIPPED, even when the report + carries incidental per-agent statuses from a partial/errored run.""" + collector = ResultCollector("dev", logger) + report = TestReport( + pks={}, + result={ + "ars": AgentReport( + status=AgentStatus.NO_RESULTS, message=None, actual_output=None + ), + "shepherd-aragorn": AgentReport( + status=AgentStatus.FAILED, message="boom", actual_output=None + ), + }, + test_details=None, + ) + collector.collect_acceptance_result( + _Case(), _Asset(), report, "pk", "http://ir/1", force_skipped=True + ) + + for agent in collector.agents: + stats = collector.acceptance_stats[agent]["TopAnswer"] + assert stats["SKIPPED"] == 1, agent + assert stats["FAILED"] == 0 and stats["NO_RESULTS"] == 0, agent + + csv_row = collector.acceptance_csv.strip().splitlines()[-1] + # every agent column is SKIPPED; nothing leaks FAILED/NO_RESULTS + assert "FAILED" not in csv_row and "NO_RESULTS" not in csv_row + assert csv_row.count("SKIPPED") == len(collector.agents) + def _performance_test_case(): return PerformanceTestCase( @@ -232,3 +271,60 @@ def test_performance_test_finished_failed_on_error(mocker): assert reporter.finished assert reporter.finished[0][1] == AgentStatus.FAILED.value + + +class _NoResponseQueryRunner(MockQueryRunner): + """Simulates a skipped test case: no query responses come back for any + asset (eg the ARS query never ran / query generation failed).""" + + def run_queries(self, test_case): + return {}, {} + + +def test_skipped_test_case_marks_all_assets_and_agents_skipped(mocker): + """When an acceptance test case is skipped, every asset must be finished + as SKIPPED in the radiator and recorded as SKIPPED for every agent in the + stats/CSV -- not FAILED for ARAs or NO_RESULTS for ARS, and never dropped + from the per-agent stats entirely.""" + mocker.patch( + "test_harness.run.QueryRunner", + return_value=_NoResponseQueryRunner(logger), + ) + + collector = ResultCollector("ci", logger) + reporter = _RecordingReporter(base_url="http://ir") + run_tests( + tests=example_test_cases, + reporter=reporter, + collector=collector, + logger=logger, + args={"suite": "acceptance", "trapi_version": "1.6.0"}, + ) + + # 3 assets total across the two acceptance cases in the fixture. + assert len(reporter.finished) == 3 + assert all(result == AgentStatus.SKIPPED.value for _, result in reporter.finished) + assert collector.acceptance_report[AgentStatus.SKIPPED.value] == 3 + assert collector.acceptance_report[AgentStatus.FAILED.value] == 0 + assert collector.acceptance_report[AgentStatus.NO_RESULTS.value] == 0 + + # Every asset shows up in the per-agent stats as SKIPPED (no ARA entry is + # silently dropped, so "0 SKIPPED" can't happen). + for agent in collector.agents: + per_agent = collector.acceptance_stats[agent] + skipped_total = sum( + per_agent[query_type][AgentStatus.SKIPPED.value] + for query_type in collector.query_types + ) + assert skipped_total == 3, f"{agent} should have 3 SKIPPED" + + # CSV has a row per asset (plus header), all SKIPPED, and labels were + # uploaded as SKIPPED for every agent on every asset. + data_rows = collector.acceptance_csv.strip().splitlines()[1:] + assert len(data_rows) == 3 + for row in data_rows: + assert "FAILED" not in row and "NO_RESULTS" not in row + assert len(reporter.labels) == 3 + for label_set in reporter.labels: + assert {label["key"] for label in label_set} == set(collector.agents) + assert all(label["value"] == AgentStatus.SKIPPED.value for label in label_set)