brbzull0 commented on code in PR #13609: URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3966747719
########## tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py: ########## @@ -0,0 +1,94 @@ +# 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: autest does not run this through a shell, so there is no shell quoting for the escaping to compensate for. `runlogic/process.py` sends a command to a shell only when `isShellCommand()` finds one of `;&><|` in it. Otherwise it parses the string with `shlex` and execs the argv directly. These `rpc invoke` commands contain none of those characters, so the pre-quoted form and a `shlex.quote()` form produce byte-identical argv: ``` old, pre-quoted "table: both" -> ['-p', 'table: both'] new, shlex.quote() -> ['-p', 'table: both'] ``` `shlex.quote()` also would not help in the one case that does reach a shell: a params value containing an operator flips `isShellCommand()` to true either way, because autest strips quotes while scanning for those characters. The escaping is unlovely, and a helper that quoted for the caller would be a fair readability change on its own. It is not a correctness one, and it would mean editing the three `get_connection_tracker_info` call sites in `traffic_ctl_server_output.test.py`, which this PR otherwise does not touch. Leaving it for separate work. ########## tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py: ########## @@ -15,10 +15,15 @@ # limitations under the License. import atexit +import json import os +import shlex import shutil import tempfile +import autest.testers as Testers +from autest.testers import All Review Comment: Measured rather than assumed: commit 7f8131e545 is exactly this import change, and its CI run was green on all four AuTest shards. `autest.testers` is importable in the harness because it *is* the harness. `core/test.py` does `import autest.testers as testers` and then injects that same module object as the `Testers` global: ```python import autest.testers as testers ... 'Testers': testers, 'All': testers.All, ``` So anything able to run a gold test can import the module, and the injected global was never anything other than the module itself. That is what made the previous stack walk unnecessary. A fallback shim would add a branch that cannot be reached. ########## 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: `GetContent()` returns a path here, and the version is pinned in-tree. `tests/pyproject.toml` pins `autest==1.10.6`, and in that release `testenities/file.py` is: ```python def GetContent(self, eventinfo): return self.AbsPath, "" ``` The tester's `TestValue` is the stdout `File` object, assigned by `core/testerset.py` when the tester is bound to `Streams.stdout`, so `tester.GetContent(info)` resolves to that file's absolute path. autest's own docstring for `Testers.Lambda` uses it the same way: ```python with open(tester.GetContent(event)) as f: ``` Since the version is pinned, the cross-version case cannot arise. Worth re-checking here if that pin ever moves. ########## 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: `GetContent()` returns a path here, and the version is pinned in-tree. `tests/pyproject.toml` pins `autest==1.10.6`, and in that release `testenities/file.py` is: ```python def GetContent(self, eventinfo): return self.AbsPath, "" ``` The tester's `TestValue` is the stdout `File` object, assigned by `core/testerset.py` when the tester is bound to `Streams.stdout`, so `tester.GetContent(info)` resolves to that file's absolute path. autest's own docstring for `Testers.Lambda` uses it the same way: ```python with open(tester.GetContent(event)) as f: ``` Since the version is pinned, the cross-version case cannot arise. Worth re-checking here if that pin ever moves. (Duplicate of the thread above; same answer.) -- 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]
