Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tools/performance_compare.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def load_summary(path, trace=None):
record = json.loads(line)
if not isinstance(record, dict):
raise ValueError("summary must be an object")
except ValueError as error:
except (ValueError, RecursionError) as error:
raise ValueError(f"{path}:{line_number}: {error}") from error
if trace is None or record.get("trace") == trace:
records.append((line_number, record))
Expand Down Expand Up @@ -182,7 +182,7 @@ def main(argv=None):
if args.require_matching_context and report["configuration_differences"]:
raise ValueError("recorded configuration differs: " + ", ".join(report["configuration_differences"]))
output = json.dumps(report, indent=2, ensure_ascii=False, allow_nan=False) + "\n" if args.json else markdown(report)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a summary whose extra is nested 2997 levels deep loads and renders as Markdown on 3.13.9 here, but --json overflows in this dumps (the report adds two container levels). The new handler turns that into exit 2, good, but the line is just performance_compare.py: maximum recursion depth exceeded ..., naming neither the input nor the report step, while the decoder path says before.jsonl:3:. raise ValueError(f"JSON report: {error}") from error around the dump, or a prefix in the handler, would make it actionable.

except (OSError, ValueError, OverflowError) as error:
except (OSError, ValueError, OverflowError, RecursionError) as error:
parser.exit(2, f"{parser.prog}: {error}\n")
sys.stdout.write(output)

Expand Down
70 changes: 70 additions & 0 deletions tools/tests/test_performance_compare.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import copy
from functools import lru_cache
import io
import json
import os
import subprocess
Expand Down Expand Up @@ -27,6 +29,19 @@
"title_id": "PPSA00000", "title": "Example", "content_version": "01.002.000"}


@lru_cache(maxsize=2)
def excessive_json_value(opening, closing):
depth = sys.getrecursionlimit() + 64
for attempt in range(8):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: attempt is never used; for _ in range(8):, as in check_conventions.py:123.

value = opening * depth + "0" + closing * depth
try:
json.loads(value)
except RecursionError:
return opening * (depth + 128) + "0" + closing * (depth + 128)
depth *= 2
raise AssertionError("JSON decoder accepted every bounded nesting probe")


class PerformanceCompareTests(unittest.TestCase):
def setUp(self):
self.directory = tempfile.TemporaryDirectory()
Expand Down Expand Up @@ -177,6 +192,61 @@ def test_percentage_overflow_is_an_error(self):
self.assertEqual(result.stdout, "")
self.assertIn("finite numeric range", result.stderr)

def test_excessive_json_nesting_reports_the_input_location(self):
for opening, closing in (("[", "]"), ('{"nested":', "}")):
with self.subTest(opening=opening):
record = json.dumps(self.baseline)[:-1] + ', "extra": ' + excessive_json_value(opening, closing) + "}"
self.before.write_text("\n\n" + record + "\n", encoding="utf-8")
with self.assertRaisesRegex(ValueError, r"before.jsonl:3:") as error:
performance_compare.load_summary(self.before)
self.assertIsInstance(error.exception.__cause__, RecursionError)

def test_excessive_json_nesting_produces_only_a_cli_error(self):
cases = ((self.before, "[", "]", ()), (self.before, '{"nested":', "}", ("--json",)),
(self.after, "[", "]", ("--json",)), (self.after, '{"nested":', "}", ()))
for path, opening, closing, args in cases:
with self.subTest(path=path.name, opening=opening, args=args):
self.write(self.before, self.baseline)
self.write(self.after, self.candidate)
record = json.dumps(self.baseline)[:-1] + ', "extra": ' + excessive_json_value(opening, closing) + "}"
path.write_text("\n\n" + record + "\n", encoding="utf-8")
original = (self.before.read_bytes(), self.after.read_bytes())
result = self.run_cli(*args)
self.assertEqual(result.returncode, 2, result.stderr)
self.assertEqual(result.stdout, "")
self.assertIn(f"{path}:3:", result.stderr)
self.assertNotIn("Traceback", result.stderr)
self.assertEqual((self.before.read_bytes(), self.after.read_bytes()), original)

def test_json_report_recursion_produces_only_a_cli_error(self):
self.write(self.before, self.baseline)
self.write(self.after, self.candidate)
with patch.object(performance_compare.json, "dumps", side_effect=RecursionError("report nesting is too deep")), \
patch.object(sys, "stdout", new_callable=io.StringIO) as output, \
patch.object(sys, "stderr", new_callable=io.StringIO) as errors:
with self.assertRaises(SystemExit) as error:
performance_compare.main([str(self.before), str(self.after), "--json"])
self.assertEqual(error.exception.code, 2)
self.assertEqual(output.getvalue(), "")
self.assertIn("report nesting is too deep", errors.getvalue())

def test_supported_nested_metadata_is_preserved(self):
metadata = {"value": 7}
for depth in range(16):
metadata = {"nested": [metadata]}
self.baseline["extra"] = metadata
self.write(self.before, self.baseline)
self.write(self.after, self.candidate)
for args in ((), ("--json",)):
with self.subTest(args=args), patch.object(sys, "stdout", new_callable=io.StringIO) as output, \
patch.object(sys, "stderr", new_callable=io.StringIO) as errors:
performance_compare.main([str(self.before), str(self.after), *args])
self.assertEqual(errors.getvalue(), "")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these assertions sit outside the with self.subTest(...) block, so a failure is reported without its args label and ends the loop; and the Markdown leg never looks at the nested metadata, since markdown() does not render extra. The test also passes on main, and any depth cap above the fixture's 33 levels would pass it too. Moving the assertions into the block and keeping the depth check on the JSON leg would make it say what its name says.

if args:
self.assertEqual(json.loads(output.getvalue())["baseline"]["extra"], metadata)
else:
self.assertIn("# Performance comparison", output.getvalue())

def test_markdown_reports_deltas_and_escapes_table_values(self):
self.candidate["frame_ms"]["max"] = 60.0
self.candidate["gpu"] = "GPU | <vendor>\nline"
Expand Down
Loading