Skip to content

Commit 5f81eb8

Browse files
committed
Merge remote-tracking branch 'origin/render/fix-loop-instrumentation' into wip/b2-sync
2 parents 5f57226 + 2759f2a commit 5f81eb8

6 files changed

Lines changed: 162 additions & 28 deletions

File tree

‎cli_output/render_summary.py‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,20 @@
1515
RENDER_TRAILER_PREFIX = "[render-trailer]"
1616

1717

18+
def render_outcome(run_state: RunState, error_message: Optional[str]) -> str:
19+
"""What the render did, for the banner and the trailer alike.
20+
21+
A reason recorded on the way out overrides the success flag. The flag is set per
22+
module, so an earlier module's success can still be standing while the process exits
23+
on an error - which read as a completed render that produced nothing.
24+
"""
25+
if run_state.render_succeeded and not error_message:
26+
return "completed"
27+
if run_state.render_cancelled:
28+
return "cancelled"
29+
return "failed"
30+
31+
1832
def print_exit_summary(
1933
run_state: RunState,
2034
spec_filename: str,
@@ -23,7 +37,7 @@ def print_exit_summary(
2337
"""Print render outcome after the TUI exits (terminal restored)."""
2438
console.quiet = False
2539

26-
if run_state.render_succeeded:
40+
if render_outcome(run_state, error_message) == "completed":
2741
msg = "\n[#79FC96]✓ rendering completed\n\n"
2842
elif run_state.render_cancelled:
2943
msg = "\n[#FFFFFF]— rendering canceled\n\n"
@@ -59,12 +73,7 @@ def log_render_trailer(
5973
the render did, and because it is written on every exit path a log *without* a
6074
trailer is itself evidence that the file was truncated.
6175
"""
62-
if run_state.render_succeeded:
63-
outcome = "completed"
64-
elif run_state.render_cancelled:
65-
outcome = "cancelled"
66-
else:
67-
outcome = "failed"
76+
outcome = render_outcome(run_state, error_message)
6877

6978
# Both times: an action that raises never reaches the render's own terminal accounting,
7079
# so the banked figure is stale on the paths this trailer exists to describe, and the

‎codeplain_REST_api.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,11 @@ def post_request(
143143
self.console.debug(f"Failed to decode JSON response: {e}. Response text: {response.text}")
144144
raise Exception(f"Error rendering plain code: Failed to decode API response ({e}).\n") from e
145145

146-
if response.status_code == requests.codes.bad_request and "error_code" in response_json:
146+
# Any status the server declines on, not only 400. The server returns
147+
# error_code with 500 as well, and reading it only on 400 meant those
148+
# reached the user as the raw "500 Server Error for url: ..." rather than
149+
# the message the code was paired with.
150+
if not response.ok and "error_code" in response_json:
147151
self._raise_for_error_code(response_json)
148152

149153
response.raise_for_status()

‎plain2code.py‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,25 @@ def warn_if_acceptance_tests_without_conformance_script(plain_module, args) -> N
192192
)
193193

194194

195+
def _render_recording_outcome(module_renderer, run_state: RunState) -> Exception | None:
196+
"""Runs the render and records how it ended, returning the failure if there was one.
197+
198+
The outcome is recorded per module, so a render that fails after an earlier module has
199+
completed is still holding that module's success. Both the render trailer and the
200+
headless exit code read that flag, and the exit code is 0 when the failure is one of
201+
EXPECTED_EXCEPTIONS - so a caller that skips this reports a failed render as a
202+
successful one.
203+
"""
204+
try:
205+
module_renderer.render_module()
206+
except RenderCancelledError:
207+
run_state.set_render_cancelled() # The TUI is already closed, nothing to report.
208+
except Exception as error:
209+
run_state.set_render_succeeded(False)
210+
return error
211+
return None
212+
213+
195214
def render( # noqa: C901
196215
plain_module: plain_modules.PlainModule,
197216
args,
@@ -285,21 +304,16 @@ def render( # noqa: C901
285304
render_error: list[Exception] = []
286305

287306
def run_render():
288-
try:
289-
module_renderer.render_module()
290-
except RenderCancelledError:
291-
run_state.set_render_cancelled() # TUI already closed, nothing to report
292-
except Exception as e:
293-
run_state.set_render_succeeded(False)
294-
render_error.append(e)
295-
event_bus.publish(RenderFailed(error_message=str(e)))
307+
error = _render_recording_outcome(module_renderer, run_state)
308+
if error is not None:
309+
render_error.append(error)
310+
event_bus.publish(RenderFailed(error_message=str(error)))
296311

297312
if args.headless:
298313
console.info(f"Render started. Render ID: {run_state.render_id}")
299-
try:
300-
module_renderer.render_module()
301-
except RenderCancelledError:
302-
run_state.set_render_cancelled()
314+
error = _render_recording_outcome(module_renderer, run_state)
315+
if error is not None:
316+
raise error
303317
return
304318
else:
305319
render_thread = threading.Thread(target=run_render, daemon=True)

‎tests/test_plain2code.py‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import tempfile
22
from argparse import Namespace
33
from types import SimpleNamespace
4-
from unittest.mock import patch
4+
from unittest.mock import MagicMock, patch
55

66
import plain2code
77
import plain_spec
8+
from plain2code_exceptions import RenderCancelledError
9+
from plain2code_state import RunState
810
from plain_modules import PlainModule
911

1012

@@ -91,3 +93,42 @@ def test_warning_covers_required_modules_for_real_plain_module(get_test_data_pat
9193
mock_console.warning.assert_called_once()
9294
warning_message = mock_console.warning.call_args.args[0]
9395
assert "required_with_acceptance_tests" in warning_message
96+
97+
98+
class TestRenderRecordsItsOutcome:
99+
"""The headless path and the TUI path both have to record how the render ended. Only
100+
the TUI path did, so a headless render that failed after one module had completed
101+
reported `outcome=completed` and, when the failure was an expected exception, exited 0.
102+
"""
103+
104+
@staticmethod
105+
def _renderer(error=None):
106+
renderer = MagicMock()
107+
if error is not None:
108+
renderer.render_module.side_effect = error
109+
return renderer
110+
111+
def test_a_failure_clears_a_success_left_by_an_earlier_module(self):
112+
run_state = RunState("reports.plain")
113+
run_state.set_render_succeeded(True)
114+
failure = RuntimeError("conformance fix loop exhausted")
115+
116+
returned = plain2code._render_recording_outcome(self._renderer(failure), run_state)
117+
118+
assert returned is failure
119+
assert run_state.render_succeeded is False
120+
121+
def test_a_clean_run_leaves_the_outcome_alone(self):
122+
run_state = RunState("reports.plain")
123+
run_state.set_render_succeeded(True)
124+
125+
assert plain2code._render_recording_outcome(self._renderer(), run_state) is None
126+
assert run_state.render_succeeded is True
127+
128+
def test_cancellation_is_recorded_as_cancelled_rather_than_returned(self):
129+
run_state = RunState("reports.plain")
130+
131+
returned = plain2code._render_recording_outcome(self._renderer(RenderCancelledError()), run_state)
132+
133+
assert returned is None
134+
assert run_state.render_cancelled is True

‎tests/test_render_trailer.py‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,15 +104,19 @@ def test_the_trailer_is_flushed_so_it_survives_an_abrupt_exit():
104104
assert handler.flush.called
105105

106106

107-
def test_a_completed_render_that_still_raised_reports_the_reason(trailer_lines):
107+
def test_a_render_that_raised_on_the_way_out_reports_the_reason_and_fails(trailer_lines):
108108
"""A render can finish its functionalities and raise on the way out — publishing the
109-
build, for instance. That combination printed a success banner, logged no reason, and
110-
exited non-zero, which is how a failure on the publish path went unnoticed across a
111-
whole run."""
109+
build, for instance. That combination printed a success banner and logged no reason,
110+
which is how a failure on the publish path went unnoticed across a whole run.
111+
112+
The reason belongs on the trailer, and so does an outcome that agrees with it: the
113+
success flag is set per module, so an earlier module's success outlives the failure of
114+
a later one, and a run that produced no build reported `outcome=completed`."""
112115
lines = trailer_lines(run_state(succeeded=True), error_message="The generated build references ...")
113116

114-
assert any("outcome=completed" in line for line in lines)
115117
assert any("error='The generated build references ...'" in line for line in lines)
118+
assert any("outcome=failed" in line for line in lines)
119+
assert not any("outcome=completed" in line for line in lines)
116120

117121

118122
def test_a_multi_line_reason_still_leaves_the_trailer_on_one_line(trailer_lines):
@@ -124,3 +128,18 @@ def test_a_multi_line_reason_still_leaves_the_trailer_on_one_line(trailer_lines)
124128
assert "\n" not in lines[0]
125129
assert "outcome=failed" in lines[0]
126130
assert "second line" in lines[0]
131+
132+
133+
class TestOutcomeAgreesWithTheReason:
134+
"""A trailer that says `completed` next to an `error=` is worse than no trailer: it is
135+
read by tooling, and it was observed on a render that produced no code at all. The
136+
success flag is set per module, so an earlier module's success outlives the failure of
137+
a later one."""
138+
139+
def test_success_with_no_error_still_completes(self, trailer_lines):
140+
assert "outcome=completed" in trailer_lines(run_state(succeeded=True))[0]
141+
142+
def test_a_cancelled_render_reports_cancelled_not_failed(self, trailer_lines):
143+
lines = trailer_lines(run_state(succeeded=False, cancelled=True), error_message="Keyboard interrupt")
144+
145+
assert "outcome=cancelled" in lines[0]

‎tests/test_rest_error_codes.py‎

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
11
"""Tests for mapping API error codes onto typed client exceptions."""
22

3-
from unittest.mock import MagicMock
3+
from unittest.mock import MagicMock, patch
44

55
import pytest
6+
import requests
67

78
import plain2code_exceptions
89
from codeplain_REST_api import CodeplainAPI
910

1011

1112
def test_conformance_fix_exhaustion_maps_to_its_typed_exception():
12-
"""The server reports fix-attempt exhaustion as a structured 400; the client raises
13+
"""The server reports fix-attempt exhaustion as a structured error; the client raises
1314
the matching typed exception so the render fails with the server's message instead
1415
of a raw HTTP error."""
1516
api = CodeplainAPI(api_key="test-key", console=MagicMock())
@@ -35,3 +36,49 @@ def test_the_exhaustion_error_is_expected_rather_than_a_crash():
3536
from plain2code import EXPECTED_EXCEPTIONS
3637

3738
assert plain2code_exceptions.ConformanceTestsFixExhausted in EXPECTED_EXCEPTIONS
39+
40+
41+
class TestErrorCodesAreReadOnEveryDeclinedStatus:
42+
"""The client used to consult `error_code` only on 400, so every structured error the
43+
server returns with a 500 - its own `InternalServerError` among them - reached the user
44+
as `500 Server Error: INTERNAL SERVER ERROR for url: <internal endpoint>`. The message
45+
the code was paired with never printed."""
46+
47+
@staticmethod
48+
def _api_returning(status, body):
49+
response = MagicMock()
50+
response.status_code = status
51+
response.ok = 200 <= status < 300
52+
response.json.return_value = body
53+
response.raise_for_status.side_effect = requests.exceptions.HTTPError(f"{status} Server Error")
54+
55+
api = CodeplainAPI(api_key="test-key", console=MagicMock())
56+
api.api_url = "https://api.example"
57+
return api, response
58+
59+
def test_a_500_carrying_an_error_code_raises_the_typed_exception(self):
60+
api, response = self._api_returning(
61+
500,
62+
{
63+
"error_code": "ConformanceTestsFixExhausted",
64+
"message": "The conformance tests for functionality 1 still failed.",
65+
},
66+
)
67+
68+
with patch("requests.post", return_value=response):
69+
with pytest.raises(plain2code_exceptions.ConformanceTestsFixExhausted, match="still failed"):
70+
api.post_request("https://api.example/x", {}, {}, None, num_retries=0)
71+
72+
def test_the_generic_server_error_reaches_the_user_as_its_own_message(self):
73+
api, response = self._api_returning(500, {"error_code": "InternalServerError", "message": "boom"})
74+
75+
with patch("requests.post", return_value=response):
76+
with pytest.raises(plain2code_exceptions.InternalServerError, match="support@codeplain.ai"):
77+
api.post_request("https://api.example/x", {}, {}, None, num_retries=0)
78+
79+
def test_a_success_is_returned_untouched(self):
80+
api, response = self._api_returning(200, {"result": "ok"})
81+
response.raise_for_status.side_effect = None
82+
83+
with patch("requests.post", return_value=response):
84+
assert api.post_request("https://api.example/x", {}, {}, None, num_retries=0) == {"result": "ok"}

0 commit comments

Comments
 (0)