brbzull0 commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3977169213
##########
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:
Re-ran this against the pinned autest rather than re-reading it.
`tester.GetContent(info)` returns a path.
A standalone test, using the same binding this PR uses -- `Testers.Lambda`
on `Streams.stdout`:
```python
def probe(info, tester):
import os
got = tester.GetContent(info)
return (os.path.isfile(got), "probe",
f"type={type(got).__name__} repr={got!r}
isfile={os.path.isfile(got)}")
tr = Test.AddTestRun("probe")
tr.Processes.Default.Command = 'printf %s \'{"partitions": []}\''
tr.Processes.Default.Streams.stdout = Testers.Lambda(probe)
```
Under `autest==1.10.6`, the version `tests/pyproject.toml` pins:
```
Passed - type=str | repr='.../0-tr-Default/stream.stdout.txt' |
os.path.isfile=True | bytes_read=b'{"partitions": []}'
```
A path to the captured stream file, which `open()` then reads. That is the
same conclusion as the resolved thread on this file, which quoted
`File.GetContent` returning `self.AbsPath, ""` and autest's own
`Testers.Lambda` docstring using `open(tester.GetContent(event))`.
The failure mode this comment predicts would also not be quiet. An exception
from a tester callback sets `KillOnFailure` and abandons the run:
```
NameError: name 'os' is not defined
autest.exceptions.killonfailure.KillOnFailureError
```
That output is from an earlier version of the probe above, which had a
genuine bug in it. The two validators run at eight call sites across
`traffic_ctl_json_null.test.py` and `traffic_ctl_server_output.test.py`, and
all four AuTest shards are green, so this branch is exercised rather than
skipped.
On the suggested alternative: there is no other API to move to. `GetContent`
is the canonical accessor for the stream filename here, and
`Streams.stdout.AbsPath` is the same string reached another way.
One note for anyone reading the thread. This review reports that Copilot
`couldn't run its full agentic review because it didn't start before the
timeout`, so it had no runner and could not read the autest source it is making
a claim about -- which is consistent with the hedging in the comment itself
("as its name suggests in some harnesses", "whatever the canonical
method/property is") and with it re-raising a thread that was already resolved
with the source quoted.
##########
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:
Same claim as the thread on `_read_stdout` above, answered there:
`tester.GetContent(info)` returns a path to the captured stream file, verified
by running a probe against the pinned `autest==1.10.6` rather than by reading
it. Resolving this duplicate.
##########
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:
Same claim as the thread on `_read_stdout` above, answered there:
`tester.GetContent(info)` returns a path to the captured stream file, verified
by running a probe against the pinned `autest==1.10.6` rather than by reading
it. Resolving this duplicate.
##########
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:
Same claim as the thread on `_read_stdout` above, answered there:
`tester.GetContent(info)` returns a path to the captured stream file, verified
by running a probe against the pinned `autest==1.10.6` rather than by reading
it. Resolving this duplicate.
--
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]
