-
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 1 commit
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,44 @@ | |
| "Ad-hoc run has no scan id to mark failed; mark_scan_as_failed must not be used. " | ||
| f"Requests seen: {request_types}" | ||
| ) | ||
|
|
||
|
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. it seems there are two failure pathways here, construction and verification, but only verification is tested, did you think about testing construction as well?
Contributor
Author
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. Good call — added |
||
|
|
||
| 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" | ||
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.
Should this be a debug log? That doesn't show up in cloud 🤔 .
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.
Agreed — bumped to
warningin 6030689. You're right that debug is the wrong level here: this only fires if the mark-scan-failed call itself fails, in which case the scan stays FAILED-with-no-logs in Cloud (exactly what this helper is trying to prevent), so it needs to be visible in the pod logs / Datadog. It can't go to Cloud by nature (Cloud reporting is what just failed), and warning keeps it from being drowned out while still not masking the original exception, which is re-raised regardless.