From cd36bc20e5b38ab9419276db356bda9d17eb3d45 Mon Sep 17 00:00:00 2001 From: Harry-Sun Date: Mon, 17 Aug 2026 16:33:01 +0800 Subject: [PATCH 1/3] fix(wren): mark truncated values in the skipped-row report _report_skipped bounded each listed value with {v!r:.120}, which cut mid-token without a marker, so a fragment was indistinguishable from a complete value. Extract the cap as _VALUE_REPR_LIMIT and report the full length alongside the cut. Also close two test holes raised in the #2570 review: the truncation test asserted only the derived '... and N more' count, which reads correctly even when the per-row listing is not capped; and the repr bound was a loose len(line) < 200 rather than the documented limit. Closes #2629 --- core/wren/src/wren/utils_cli.py | 20 +++++++++- core/wren/tests/unit/test_type_mapping.py | 46 +++++++++++++++++++++-- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/core/wren/src/wren/utils_cli.py b/core/wren/src/wren/utils_cli.py index 27e5ad9958..18983e09f6 100644 --- a/core/wren/src/wren/utils_cli.py +++ b/core/wren/src/wren/utils_cli.py @@ -17,6 +17,22 @@ # _WARNING_SUMMARY_THRESHOLD so large batches don't flood stderr. _SKIP_REPORT_LIMIT = 10 +# Cap on the rendered length of a single listed value. Past this the repr is +# cut and marked, so a fragment is never mistaken for a complete value. +_VALUE_REPR_LIMIT = 120 + + +def _format_corrupt_value(value: object) -> str: + """Render ``value``'s repr, marking it when the length cap cuts it. + + An unmarked cut reads exactly like a short value that happens to end + there, so a cut repr also reports the full length it was cut from. + """ + rendered = repr(value) + if len(rendered) <= _VALUE_REPR_LIMIT: + return rendered + return f"{rendered[:_VALUE_REPR_LIMIT]}... ({len(rendered)} chars total)" + def _skipped_rows(data: object) -> list[tuple[int, object]]: """Return (index, value) for entries parse_types/translate_types will drop. @@ -53,7 +69,9 @@ def _report_skipped(skipped: list[tuple[int, object]]) -> None: err=True, ) for i, v in corrupt[:_SKIP_REPORT_LIMIT]: - typer.echo(f" [{i}] {type(v).__name__}: {v!r:.120}", err=True) + typer.echo( + f" [{i}] {type(v).__name__}: {_format_corrupt_value(v)}", err=True + ) remaining = len(corrupt) - _SKIP_REPORT_LIMIT if remaining > 0: typer.echo(f" ... and {remaining} more", err=True) diff --git a/core/wren/tests/unit/test_type_mapping.py b/core/wren/tests/unit/test_type_mapping.py index d905fffcb5..5061707055 100644 --- a/core/wren/tests/unit/test_type_mapping.py +++ b/core/wren/tests/unit/test_type_mapping.py @@ -14,6 +14,7 @@ translate_type, translate_types, ) +from wren.utils_cli import _VALUE_REPR_LIMIT # ── parse_type unit tests ────────────────────────────────────────────────── @@ -452,18 +453,41 @@ def test_cli_parse_types_skip_report_truncates_past_limit() -> None: assert "Warning: skipped 12 non-mapping row(s)" in result.stderr assert "[1] int: 0" in result.stderr assert "... and 2 more" in result.stderr + # The trailing count is derived from _SKIP_REPORT_LIMIT, so it reads + # correctly even if the per-row listing was never capped. Pin the cap + # itself: row 11 is the first past the limit and must not be listed. + assert "[11] int: 10" not in result.stderr def test_cli_parse_types_corrupt_value_repr_is_bounded() -> None: # A few large corrupt values must not flood stderr just because there are - # fewer of them than _SKIP_REPORT_LIMIT. - columns = [{"column": "id", "raw_type": "int8"}, "x" * 5000] + # fewer of them than _SKIP_REPORT_LIMIT. The cut must also be visible, so a + # fragment is never read as the whole value. + raw = "x" * 5000 + columns = [{"column": "id", "raw_type": "int8"}, raw] result = _run_wren( "utils", "parse-types", "--dialect", "postgres", stdin=json.dumps(columns) ) _assert_success(result) line = next(ln for ln in result.stderr.splitlines() if ln.startswith(" [1]")) - assert len(line) < 200 + rendered = repr(raw) + assert line == ( + f" [1] str: {rendered[:_VALUE_REPR_LIMIT]}... ({len(rendered)} chars total)" + ) + + +def test_cli_parse_types_corrupt_value_repr_under_limit_is_unmarked() -> None: + # A value that fits the cap must render exactly as its repr, with no + # truncation marker to imply something was withheld. + raw = "short-but-wrong" + columns = [{"column": "id", "raw_type": "int8"}, raw] + result = _run_wren( + "utils", "parse-types", "--dialect", "postgres", stdin=json.dumps(columns) + ) + _assert_success(result) + line = next(ln for ln in result.stderr.splitlines() if ln.startswith(" [1]")) + assert line == f" [1] str: {raw!r}" + assert "chars total" not in line def test_cli_translate_types_strict_exits_nonzero_on_corrupt_row() -> None: @@ -534,3 +558,19 @@ def test_cli_translate_types_rejects_non_list_object_payload() -> None: assert result.returncode == 1 assert "Error: input must be a JSON array (got dict)" in result.stderr assert "AssertionError" not in result.stderr + + +def test_cli_translate_types_rejects_non_list_string_payload() -> None: + # Mirrors the parse-types string-payload regression; both commands go + # through the same _require_list guard. + result = _run_wren( + "utils", + "translate-types", + "--source", + "postgres", + "--target", + "bigquery", + stdin=json.dumps("not-a-list"), + ) + assert result.returncode == 1 + assert "Error: input must be a JSON array (got str)" in result.stderr From 488334e3c376b3a1a7352f9e51e6282fa592ad93 Mon Sep 17 00:00:00 2001 From: Harry-Sun Date: Mon, 17 Aug 2026 17:13:06 +0800 Subject: [PATCH 2/3] test(wren): assert clean failure on translate-types string payload A non-list payload must error cleanly rather than crash, so pin the absence of a traceback alongside the exit code and message. Addresses CodeRabbit review on #2675. --- core/wren/tests/unit/test_type_mapping.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/wren/tests/unit/test_type_mapping.py b/core/wren/tests/unit/test_type_mapping.py index 5061707055..68d4214b3e 100644 --- a/core/wren/tests/unit/test_type_mapping.py +++ b/core/wren/tests/unit/test_type_mapping.py @@ -574,3 +574,5 @@ def test_cli_translate_types_rejects_non_list_string_payload() -> None: ) assert result.returncode == 1 assert "Error: input must be a JSON array (got str)" in result.stderr + assert "Traceback (most recent call last)" not in result.stderr + assert "AssertionError" not in result.stderr From 2cd15cf70f1addec3e6191a9e7ba2a2ff942a3be Mon Sep 17 00:00:00 2001 From: Harry-Sun Date: Mon, 17 Aug 2026 17:23:48 +0800 Subject: [PATCH 3/3] test(wren): pin _VALUE_REPR_LIMIT so the cap cannot drift silently The rendering tests derive their expectations from the constant, which keeps them correct at any cap but lets the value itself change unnoticed. Pin it in one dedicated test instead of hardcoding the number alongside each expectation. Addresses Copilot review on #2675. --- core/wren/tests/unit/test_type_mapping.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/wren/tests/unit/test_type_mapping.py b/core/wren/tests/unit/test_type_mapping.py index 68d4214b3e..a7159c0840 100644 --- a/core/wren/tests/unit/test_type_mapping.py +++ b/core/wren/tests/unit/test_type_mapping.py @@ -459,6 +459,14 @@ def test_cli_parse_types_skip_report_truncates_past_limit() -> None: assert "[11] int: 10" not in result.stderr +def test_value_repr_limit_is_the_documented_cap() -> None: + # The cap is a display knob, but moving it changes user-visible output, so + # it should take a deliberate edit rather than drift silently. The + # rendering tests below derive their expectations from the constant, so + # this is the one place the value itself is pinned. + assert _VALUE_REPR_LIMIT == 120 + + def test_cli_parse_types_corrupt_value_repr_is_bounded() -> None: # A few large corrupt values must not flood stderr just because there are # fewer of them than _SKIP_REPORT_LIMIT. The cut must also be visible, so a