Copilot commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3956951988
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,97 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+# Names autest injects into every test file's globals, which this module needs.
+_INJECTED_NAMES = ('Testers', 'All')
+
+
+def _test_file_globals():
+ """Return the globals of the calling test file.
+
+ autest injects the names in `_INJECTED_NAMES` into each test file's
+ globals rather than exposing them for import, so a helper module has to
+ reach up the stack to find them. The search walks outward until it
+ reaches a frame carrying all of them, rather than assuming the immediate
+ caller is the test file. That way it works from inside this module and
+ from any intermediate helper module, and a frame that carries only some
+ of the names cannot satisfy the search and fail later on the rest.
+ """
+ frame = sys._getframe(1)
+ while frame is not None and not all(name in frame.f_globals for name in
_INJECTED_NAMES):
+ frame = frame.f_back
+ if frame is None:
+ raise RuntimeError(
+ f"No autest test file frame found. These helpers only work when
called from a test file, "
+ f"whose globals carry {', '.join(_INJECTED_NAMES)}.")
+ return frame.f_globals
+
+
+def _read_stdout(path):
+ """Read a captured stream file as UTF-8.
+
+ JSON is defined to be UTF-8, and naming the encoding keeps the decode
+ from following the runner's locale: under `LC_ALL=C` the default is
+ US-ASCII, so identical output bytes would decode differently there.
+
+ Undecodable bytes are replaced rather than raising. autest treats an
+ exception from a tester callback as fatal, setting KillOnFailure and
+ abandoning the rest of the test run, whereas a replaced byte simply
+ fails the JSON parse and is reported with the output attached.
+ """
+ with open(path, encoding='utf-8', errors='replace') as stream:
+ return stream.read()
+
+
+def _check_is_valid_json(path):
+ """Tester callback: the captured output must parse as JSON."""
+ desc = "Check that the output parses as JSON"
+ raw = _read_stdout(path)
+ try:
Review Comment:
`errors='replace'` can allow invalid UTF-8 output to still parse as JSON
(after replacement), which can produce false positives for “valid JSON”. Since
JSON text must be UTF-8, consider reading bytes and decoding with strict UTF-8,
catching `UnicodeDecodeError` and returning a tester failure tuple (including
the raw output or a replacement-rendering for diagnostics) rather than
replacing bytes up-front.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,97 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+# Names autest injects into every test file's globals, which this module needs.
+_INJECTED_NAMES = ('Testers', 'All')
+
+
+def _test_file_globals():
+ """Return the globals of the calling test file.
+
+ autest injects the names in `_INJECTED_NAMES` into each test file's
+ globals rather than exposing them for import, so a helper module has to
+ reach up the stack to find them. The search walks outward until it
+ reaches a frame carrying all of them, rather than assuming the immediate
+ caller is the test file. That way it works from inside this module and
+ from any intermediate helper module, and a frame that carries only some
+ of the names cannot satisfy the search and fail later on the rest.
+ """
+ frame = sys._getframe(1)
+ while frame is not None and not all(name in frame.f_globals for name in
_INJECTED_NAMES):
+ frame = frame.f_back
+ if frame is None:
+ raise RuntimeError(
+ f"No autest test file frame found. These helpers only work when
called from a test file, "
+ f"whose globals carry {', '.join(_INJECTED_NAMES)}.")
Review Comment:
`_test_file_globals()` currently requires both `Testers` and `All` to be
present even for call sites that only need `Testers` (e.g.,
`validate_json_contains` / `validate_is_valid_json`). This makes the helper
more brittle than necessary. Consider allowing
`_test_file_globals(required_names=...)` and passing only the names needed per
call site (keeping the stricter requirement where `All` is actually used).
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,97 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+# Names autest injects into every test file's globals, which this module needs.
+_INJECTED_NAMES = ('Testers', 'All')
+
+
+def _test_file_globals():
+ """Return the globals of the calling test file.
+
+ autest injects the names in `_INJECTED_NAMES` into each test file's
+ globals rather than exposing them for import, so a helper module has to
+ reach up the stack to find them. The search walks outward until it
+ reaches a frame carrying all of them, rather than assuming the immediate
+ caller is the test file. That way it works from inside this module and
+ from any intermediate helper module, and a frame that carries only some
+ of the names cannot satisfy the search and fail later on the rest.
+ """
+ frame = sys._getframe(1)
+ while frame is not None and not all(name in frame.f_globals for name in
_INJECTED_NAMES):
+ frame = frame.f_back
+ if frame is None:
+ raise RuntimeError(
+ f"No autest test file frame found. These helpers only work when
called from a test file, "
+ f"whose globals carry {', '.join(_INJECTED_NAMES)}.")
Review Comment:
`_test_file_globals()` currently requires both `Testers` and `All` to be
present even for call sites that only need `Testers` (e.g.,
`validate_json_contains` / `validate_is_valid_json`). This makes the helper
more brittle than necessary. Consider allowing
`_test_file_globals(required_names=...)` and passing only the names needed per
call site (keeping the stricter requirement where `All` is actually used).
--
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]
