Copilot commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3951415278
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,56 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+def _test_file_globals():
+ """Return the globals of the calling test file.
+
+ autest injects `Testers` and `All` 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
+ that carries the injected names, rather than assuming the immediate
+ caller is the test file. That way it works from inside this module and
+ from any intermediate helper module.
+ """
+ frame = sys._getframe(1)
+ while frame is not None and 'Testers' not in frame.f_globals:
+ frame = frame.f_back
+ if frame is None:
+ raise RuntimeError('No autest test file frame found. These helpers
only work when called from a test file.')
Review Comment:
`_test_file_globals()` only searches for `'Testers'`, but
`validate_contains_all()` immediately also expects `'All'`. If a frame contains
`Testers` but not `All`, this will fail later with a `KeyError` that’s harder
to diagnose. Update the search condition to require both injected names (e.g.,
look for both `'Testers'` and `'All'`) and adjust the error message accordingly.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py:
##########
@@ -0,0 +1,84 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import sys
+
+# To include util classes
+sys.path.insert(0, f'{Test.TestDirectory}')
+
+from traffic_ctl_test_utils import Make_traffic_ctl
+
+Test.Summary = '''
+traffic_ctl JSON output must be parseable JSON, including when a node is null.
+
+yaml-cpp emits null as `~`, which is valid YAML but rejected by every JSON
+parser. Emitters that produce JSON must set YAML::LowerNull, and container
+nodes that may stay empty must be constructed as sequences so they emit `[]`
+rather than null.
+
+The trigger for both regressions is the *empty* case, so this test
+deliberately runs against a freshly started server with an empty HostDB and no
+plugins loaded. A test that populates either one first would pass against the
+bug.
+'''
+
+Test.ContinueOnFail = True
+
+records_yaml = '''
+ exec_thread:
+ autoconfig:
+ enabled: 0
+ limit: 4
+ '''
+
+traffic_ctl = Make_traffic_ctl(Test, records_yaml)
+
+######
+# hostdb status -- `partitions` is empty on a fresh server.
+#
+# Flagless output goes through BasePrinter::write_output_json (the client
+# printer). Before the fix this emitted `"partitions": ~`.
+traffic_ctl.hostdb().status().validate_is_valid_json()
+
+# ... and it must be an empty array, not null. hostdb_status_schema.json
+# declares partitions as "type": "array".
+traffic_ctl.hostdb().status().validate_json_contains(partitions='[]')
+
+# -f json goes through the full envelope. Same emitter, different entry point.
+traffic_ctl.hostdb().status().as_json().validate_is_valid_json()
+
+# The server-side encoder (yamlcpp_json_encoder) is a third, independent
+# emitter. rpc invoke exercises it directly.
+#
+# The params are required: get_hostdb_status without them fails with "invalid
+# node; this may result from using a map iterator as a sequence iterator", and
+# an error envelope is valid JSON no matter what the emitter does -- the
+# assertion would pass against the bug.
+traffic_ctl.rpc().invoke(handler="get_hostdb_status", params='"hostname:
\\"\\""').validate_is_valid_json()
Review Comment:
This relies on manually pre-quoting the `-p` argument to survive shell
parsing, which is fragile and easy to break if the params need spaces/newlines.
Since this PR already introduces `shlex.quote()` for `hostdb status`, it would
be more robust to have `RPC.invoke()` shell-quote `params` (or accept a
structured type and serialize it), and then update this test to pass an
unquoted value.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,56 @@ def MakeGoldFileWithText(content, dir, test_number,
add_new_line=True):
return gold_filepath
+def _test_file_globals():
+ """Return the globals of the calling test file.
+
+ autest injects `Testers` and `All` 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
+ that carries the injected names, rather than assuming the immediate
+ caller is the test file. That way it works from inside this module and
+ from any intermediate helper module.
+ """
+ frame = sys._getframe(1)
+ while frame is not None and 'Testers' not in frame.f_globals:
+ frame = frame.f_back
+ if frame is None:
+ raise RuntimeError('No autest test file frame found. These helpers
only work when called from a test file.')
+ return frame.f_globals
+
+
+def _read_stdout(path):
+ """Read a captured stream file, tolerating output that is not valid
UTF-8."""
+ with open(path, errors='replace') as stream:
+ return stream.read()
Review Comment:
Reading in text mode without an explicit encoding makes decoding
locale-dependent, which can cause JSON parsing to behave differently across
environments (even if the output bytes are identical). Consider reading bytes
and decoding explicitly as UTF-8 (JSON’s required encoding), choosing
`errors='strict'` to enforce valid JSON or `errors='replace'` if you
intentionally want lossy diagnostics—either way making the behavior
deterministic.
--
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]
