-
Notifications
You must be signed in to change notification settings - Fork 278
fix(check-collections): report runner scan FAILED with logs on abort re-raise (SAS-13001) #2779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,10 @@ | |
|
|
||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
| from helpers.mock_soda_cloud import MockSodaCloud | ||
| from soda_core.common.data_source_impl import DataSourceImpl | ||
| from soda_core.common.logging_constants import soda_logger | ||
| from soda_core.common.yaml import ContractYamlSource, DataSourceYamlSource | ||
| from soda_core.contracts.contract_verification import ContractVerificationSession | ||
| from soda_core.contracts.impl.contract_verification_impl import ContractImpl | ||
|
|
@@ -145,3 +147,83 @@ | |
| "Ad-hoc run has no scan id to mark failed; mark_scan_as_failed must not be used. " | ||
| f"Requests seen: {request_types}" | ||
| ) | ||
|
|
||
|
|
||
| def test_uncaught_exception_during_verify_marks_scan_failed_with_logs(monkeypatch): | ||
| """A single-contract (runner) scan that raises an *uncaught* exception during verify aborts | ||
| and re-raises before phase 3's combined upload runs. The engine must still report the runner | ||
| scan as FAILED with the captured logs, otherwise the Cloud scan record shows no logs and the | ||
| failure is undiagnosable (SAS-13001).""" | ||
| monkeypatch.setenv("SODA_SCAN_ID", "scan-under-test") | ||
|
|
||
| def _boom(self, *args, **kwargs): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love the name 💥 |
||
| soda_logger.error("Boom: could not build check collection") | ||
| raise RuntimeError("verify exploded") | ||
|
|
||
| monkeypatch.setattr(ContractImpl, "verify", _boom) | ||
|
|
||
| data_source_impl = DataSourceImpl.from_yaml_source(DataSourceYamlSource.from_str(_DATA_SOURCE_YAML)) | ||
| mock_cloud = MockSodaCloud() | ||
| mock_cloud._upload_contract_yaml_file = lambda *args, **kwargs: "contract-file-id" | ||
|
|
||
| # The abort-on-first-error contract path re-raises the exception verbatim. | ||
| with pytest.raises(RuntimeError, match="verify exploded"): | ||
|
Check warning on line 170 in soda-tests/tests/unit/test_contract_marks_scan_failed_on_connection_error.py
|
||
| ContractVerificationSession.execute( | ||
| contract_yaml_sources=[ContractYamlSource.from_str(_CONTRACT_YAML)], | ||
| data_source_impls=[data_source_impl], | ||
| soda_cloud_impl=mock_cloud, | ||
| soda_cloud_publish_results=True, | ||
| ) | ||
|
|
||
| request_types = [r.json.get("type") for r in mock_cloud.requests if isinstance(r.json, dict)] | ||
| mark_requests = [ | ||
| r.json | ||
| for r in mock_cloud.requests | ||
| if isinstance(r.json, dict) and r.json.get("type") == "sodaCoreMarkScanFailed" | ||
| ] | ||
| assert mark_requests, ( | ||
| "A scan that raised an uncaught exception during verify must be marked FAILED so the " | ||
| f"failure is visible in Cloud. Requests seen: {request_types}" | ||
| ) | ||
| assert mark_requests[0].get("scanId") == "scan-under-test" | ||
| # The captured engine logs must be shipped, not an empty payload — that is the whole point. | ||
| assert mark_requests[0].get("logs"), "mark-scan-failed must carry the captured engine logs, not an empty payload" | ||
|
|
||
|
|
||
| def test_uncaught_exception_during_construction_marks_scan_failed_with_logs(monkeypatch): | ||
| """The other abort pathway: an uncaught exception during contract *construction* (phase 1, before | ||
| verify) must likewise mark the runner scan FAILED with the captured logs before re-raising. Both | ||
| the construction and verify abort points call the same reporting helper (SAS-13001).""" | ||
| monkeypatch.setenv("SODA_SCAN_ID", "scan-under-test") | ||
|
|
||
| def _boom_init(self, *args, **kwargs): | ||
| soda_logger.error("Boom: could not construct contract") | ||
| raise RuntimeError("construction exploded") | ||
|
|
||
| # Fail inside ContractImpl.__init__ so the exception escapes phase-1 construction, not verify(). | ||
| monkeypatch.setattr(ContractImpl, "__init__", _boom_init) | ||
|
|
||
| data_source_impl = DataSourceImpl.from_yaml_source(DataSourceYamlSource.from_str(_DATA_SOURCE_YAML)) | ||
| mock_cloud = MockSodaCloud() | ||
| mock_cloud._upload_contract_yaml_file = lambda *args, **kwargs: "contract-file-id" | ||
|
|
||
| with pytest.raises(RuntimeError, match="construction exploded"): | ||
|
Check warning on line 210 in soda-tests/tests/unit/test_contract_marks_scan_failed_on_connection_error.py
|
||
| ContractVerificationSession.execute( | ||
| contract_yaml_sources=[ContractYamlSource.from_str(_CONTRACT_YAML)], | ||
| data_source_impls=[data_source_impl], | ||
| soda_cloud_impl=mock_cloud, | ||
| soda_cloud_publish_results=True, | ||
| ) | ||
|
|
||
| request_types = [r.json.get("type") for r in mock_cloud.requests if isinstance(r.json, dict)] | ||
| mark_requests = [ | ||
| r.json | ||
| for r in mock_cloud.requests | ||
| if isinstance(r.json, dict) and r.json.get("type") == "sodaCoreMarkScanFailed" | ||
| ] | ||
| assert mark_requests, ( | ||
| "A scan that raised an uncaught exception during construction must be marked FAILED. " | ||
| f"Requests seen: {request_types}" | ||
| ) | ||
| assert mark_requests[0].get("scanId") == "scan-under-test" | ||
| assert mark_requests[0].get("logs"), "mark-scan-failed must carry the captured engine logs, not an empty payload" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems there are two failure pathways here, construction and verification, but only verification is tested, did you think about testing construction as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — added
test_uncaught_exception_during_construction_marks_scan_failed_with_logsin 6030689 (raises insideContractImpl.__init__so the exception escapes phase-1 construction rather than verify). Both abort points call the same helper, so this locks in that the construction pathway reports FAILED-with-logs too. 979 unit tests green.