Copilot commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3976945034
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +66,93 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+def _read_stdout(path):
Review Comment:
`_check_is_valid_json` / `_check_json_fields` treat their argument as a
filesystem path (they call `open(path, ...)`), but the Lambda passes
`tester.GetContent(info)`. If `GetContent` returns the stream *contents* (as
its name suggests in some harnesses), this will try to open that content as a
filename and fail the test run (likely as a fatal exception). Prefer passing
the actual captured stdout file path from the autest API (whatever the
canonical method/property is for “stream filename”), or update the helpers to
accept raw content bytes/text instead of a path.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +66,93 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+def _read_stdout(path):
+ """Read a captured stream file as strict UTF-8, as `(text, error)`.
+
+ JSON has to be UTF-8, so output that does not decode is not valid JSON
+ either and the callers report it as a failure. Replacing the bad bytes
+ instead would hide exactly that: U+FFFD is a legal character inside a
+ JSON string, so undecodable output would go on to parse cleanly and pass
+ a check whose whole job is to reject output that is not JSON.
+
+ No command wrapped here reaches that branch today. yaml-cpp substitutes
+ U+FFFD itself when writing a double quoted scalar, so traffic_ctl cannot
+ put undecodable bytes on stdout on any of these paths. Decoding strictly
+ states the requirement rather than resting on that staying true.
+
+ The failure comes back as a value rather than an exception because autest
+ treats an exception from a tester callback as fatal, setting KillOnFailure
+ and abandoning the rest of the test run. On failure the text is still
+ rendered, lossily, so the caller can show what arrived.
+ """
+ with open(path, 'rb') as stream:
+ raw = stream.read()
Review Comment:
`_check_is_valid_json` / `_check_json_fields` treat their argument as a
filesystem path (they call `open(path, ...)`), but the Lambda passes
`tester.GetContent(info)`. If `GetContent` returns the stream *contents* (as
its name suggests in some harnesses), this will try to open that content as a
filename and fail the test run (likely as a fatal exception). Prefer passing
the actual captured stdout file path from the autest API (whatever the
canonical method/property is for “stream filename”), or update the helpers to
accept raw content bytes/text instead of a path.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +66,93 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+def _read_stdout(path):
+ """Read a captured stream file as strict UTF-8, as `(text, error)`.
+
+ JSON has to be UTF-8, so output that does not decode is not valid JSON
+ either and the callers report it as a failure. Replacing the bad bytes
+ instead would hide exactly that: U+FFFD is a legal character inside a
+ JSON string, so undecodable output would go on to parse cleanly and pass
+ a check whose whole job is to reject output that is not JSON.
+
+ No command wrapped here reaches that branch today. yaml-cpp substitutes
+ U+FFFD itself when writing a double quoted scalar, so traffic_ctl cannot
+ put undecodable bytes on stdout on any of these paths. Decoding strictly
+ states the requirement rather than resting on that staying true.
+
+ The failure comes back as a value rather than an exception because autest
+ treats an exception from a tester callback as fatal, setting KillOnFailure
+ and abandoning the rest of the test run. On failure the text is still
+ rendered, lossily, so the caller can show what arrived.
+ """
+ with open(path, 'rb') as stream:
+ raw = stream.read()
+ try:
+ return raw.decode('utf-8'), None
+ except UnicodeDecodeError as ex:
+ return raw.decode('utf-8', errors='replace'), str(ex)
+
+
+def _check_is_valid_json(path):
+ """Tester callback: the captured output must parse as JSON."""
+ desc = "Check that the output parses as JSON"
+ raw, decode_error = _read_stdout(path)
+ if decode_error:
+ return (False, desc, f"Output is not valid UTF-8, so it is not JSON:
{decode_error}\nOutput was:\n{raw}")
+ try:
+ json.loads(raw)
+ except ValueError as ex:
+ return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}")
+ return (True, desc, "Output parses as JSON")
+
+
+def _check_json_fields(path, expected):
+ """Tester callback: every expected field must match its value in the
parsed output.
+
+ The expectation is compared against `str()` of the parsed value, so write
+ it the way Python renders that value rather than the way JSON spells it.
+ `[]` and `'[]'` agree, but a list of strings renders as `"['a']"`, not
+ `'["a"]'`.
+
+ Booleans need care, because the emitters these tests cover set
+ `YAML::DoubleQuoted` and so encode every scalar as a JSON string.
+ `get_server_status` sends `"is_draining": "false"`, which parses to the
+ string `'false'` and is matched by `is_draining='false'`. A genuine JSON
+ boolean would instead parse to Python `True` or `False` and render as
+ `'True'` or `'False'`.
+
+ `validate_result_with_text` does take JSON-spelled text, so the two
+ helpers are not interchangeable.
+
+ A key the output does not carry is reported as missing rather than
+ compared, so a misspelled field name fails instead of quietly matching an
+ expectation of `None`. Asserting that a field is present and null is
+ therefore written `field=None`, which only passes when the key is there.
+ """
+ desc = "Check that the JSON output contains the expected fields"
+ raw, decode_error = _read_stdout(path)
+ if decode_error:
+ return (False, desc, f"Output is not valid UTF-8, so it is not JSON:
{decode_error}\nOutput was:\n{raw}")
+ try:
+ doc = json.loads(raw)
+ except ValueError as ex:
+ return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}")
+ if not isinstance(doc, dict):
+ return (False, desc, f"Output is a JSON {type(doc).__name__}, not an
object, so it has no fields\nOutput was:\n{raw}")
+
+ failed = []
+ for key, want in expected.items():
+ if key not in doc:
+ failed.append(f"{key} is missing (expected {want})")
+ continue
+ actual = doc[key]
+ if str(actual) != str(want):
+ failed.append(f"{key} = {actual} (expected {want})")
Review Comment:
Comparing `str(actual)` to `str(want)` can mask meaningful JSON type
regressions (e.g., `1` vs `'1'`, `true` vs `'true'`), which is especially
relevant given the PR’s goal of tightening JSON correctness. A more robust
approach is to compare values directly and only apply string coercion for the
specific legacy cases you’ve documented (or provide an explicit opt-in flag for
string-coercive comparisons).
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -142,26 +229,42 @@ def validate_result_with_text(self, text: str):
def validate_json_contains(self, **field_checks):
"""
Validate JSON output contains specific field:value pairs. Only checks
specified fields.
- Prints detailed error on failure: "FAIL: field_name = actual_value
(expected expected_value)"
- stream.all.txt will contain the actual output with the failed fields.
+ Every mismatch is reported as "field_name = actual_value (expected
expected_value)",
+ followed by the raw output.
+
+ The check runs in the autest process against the captured stdout file.
Piping
+ traffic_ctl into a JSON parser instead would hide failures: the exit
status of a shell
+ pipeline is the parser's, so a non-zero traffic_ctl exit would never
reach the
+ ReturnCode check.
Example:
traffic_ctl.server().status().validate_json_contains(
initialized_done='true', is_draining='false'
)
"""
- import json
- checks_str = ', '.join(f"'{k}': '{v}'" for k, v in
field_checks.items())
- self._cmd = (
- f'{self._cmd} | python3 -c "'
- f"import sys, json; "
- f"d = json.load(sys.stdin); "
- f"c = {{{checks_str}}}; "
- f"failed = [(k, v, str(d.get(k))) for k, v in c.items() if
str(d.get(k)) != v]; "
- f"[print(f'FAIL: {{k}} = {{actual}} (expected {{expected}})',
file=sys.stderr) "
- f"for k, expected, actual in failed]; "
- f"exit(0 if not failed else 1)"
- f'"')
+ self._tr.Processes.Default.Streams.stdout = Testers.Lambda(
+ lambda info, tester: _check_json_fields(tester.GetContent(info),
field_checks))
+ self._finish()
+ return self
+
+ def validate_is_valid_json(self):
+ """
+ Validate that stdout parses as JSON. Performs no field checks.
+
+ Use this as a regression guard on any command documented to emit JSON.
+ A gold file cannot do this job: yaml-cpp spells null as `~`, which a
+ gold file matches happily but no JSON parser accepts.
+
+ The check runs in the autest process against the captured stdout file,
so
+ traffic_ctl stays the only process in the test run and the exit status
the
+ harness compares against ReturnCode is still traffic_ctl's own. The raw
+ output is reported on failure.
+
+ Example:
+ traffic_ctl.hostdb().status().validate_is_valid_json()
+ """
+ self._tr.Processes.Default.Streams.stdout = Testers.Lambda(
+ lambda info, tester: _check_is_valid_json(tester.GetContent(info)))
Review Comment:
`_check_is_valid_json` / `_check_json_fields` treat their argument as a
filesystem path (they call `open(path, ...)`), but the Lambda passes
`tester.GetContent(info)`. If `GetContent` returns the stream *contents* (as
its name suggests in some harnesses), this will try to open that content as a
filename and fail the test run (likely as a fatal exception). Prefer passing
the actual captured stdout file path from the autest API (whatever the
canonical method/property is for “stream filename”), or update the helpers to
accept raw content bytes/text instead of a path.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -142,26 +229,42 @@ def validate_result_with_text(self, text: str):
def validate_json_contains(self, **field_checks):
"""
Validate JSON output contains specific field:value pairs. Only checks
specified fields.
- Prints detailed error on failure: "FAIL: field_name = actual_value
(expected expected_value)"
- stream.all.txt will contain the actual output with the failed fields.
+ Every mismatch is reported as "field_name = actual_value (expected
expected_value)",
+ followed by the raw output.
+
+ The check runs in the autest process against the captured stdout file.
Piping
+ traffic_ctl into a JSON parser instead would hide failures: the exit
status of a shell
+ pipeline is the parser's, so a non-zero traffic_ctl exit would never
reach the
+ ReturnCode check.
Example:
traffic_ctl.server().status().validate_json_contains(
initialized_done='true', is_draining='false'
)
"""
- import json
- checks_str = ', '.join(f"'{k}': '{v}'" for k, v in
field_checks.items())
- self._cmd = (
- f'{self._cmd} | python3 -c "'
- f"import sys, json; "
- f"d = json.load(sys.stdin); "
- f"c = {{{checks_str}}}; "
- f"failed = [(k, v, str(d.get(k))) for k, v in c.items() if
str(d.get(k)) != v]; "
- f"[print(f'FAIL: {{k}} = {{actual}} (expected {{expected}})',
file=sys.stderr) "
- f"for k, expected, actual in failed]; "
- f"exit(0 if not failed else 1)"
- f'"')
+ self._tr.Processes.Default.Streams.stdout = Testers.Lambda(
+ lambda info, tester: _check_json_fields(tester.GetContent(info),
field_checks))
Review Comment:
`_check_is_valid_json` / `_check_json_fields` treat their argument as a
filesystem path (they call `open(path, ...)`), but the Lambda passes
`tester.GetContent(info)`. If `GetContent` returns the stream *contents* (as
its name suggests in some harnesses), this will try to open that content as a
filename and fail the test run (likely as a fatal exception). Prefer passing
the actual captured stdout file path from the autest API (whatever the
canonical method/property is for “stream filename”), or update the helpers to
accept raw content bytes/text instead of a path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
