From 8f534f8711049af50fda5f866c6854533b09a477 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 3 Sep 2026 13:23:11 +0200 Subject: [PATCH 1/3] traffic_ctl: honor -f json for plugin list plugin_list() formatted its table straight to std::cout and only consulted the printer on the error path, so --format was silently ignored on success and the output could not be consumed as JSON. Move the table into a PluginListPrinter so the command follows the same path as every other one: the base class dispatches on format, emitting the JSON-RPC envelope for -f json and delegating to the printer for text. Text output is unchanged. --- src/traffic_ctl/CtrlCommands.cc | 47 +++++---------------------------- src/traffic_ctl/CtrlPrinters.cc | 40 ++++++++++++++++++++++++++++ src/traffic_ctl/CtrlPrinters.h | 8 ++++++ 3 files changed, 55 insertions(+), 40 deletions(-) diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 8b880830672..97d5292d43a 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -1004,12 +1003,17 @@ HostDBCommand::status_get() //------------------------------------------------------------------------------------------------------------------------------------ PluginCommand::PluginCommand(ts::Arguments *args) : CtrlCommand(args) { + BasePrinter::Options printOpts{parse_print_opts(args)}; + if (get_parsed_arguments()->get(MSG_STR)) { _invoked_func = [&]() { plugin_msg(); }; + _printer = std::make_unique(printOpts); } else if (get_parsed_arguments()->get(LIST_STR)) { _invoked_func = [&]() { plugin_list(); }; + _printer = std::make_unique(printOpts); + } else { + _printer = std::make_unique(printOpts); } - _printer = std::make_unique(parse_print_opts(args)); } void @@ -1033,44 +1037,7 @@ PluginCommand::plugin_list() GetPluginListRequest request; auto response = invoke_rpc(request); - if (response.is_error()) { - _printer->write_output(response); - return; - } - - auto info = response.result.as(); - - std::cout << "source: " << info.source << '\n'; - - bool has_load_order = false; - for (const auto &p : info.plugins) { - if (p.load_order >= 0) { - has_load_order = true; - break; - } - } - - if (has_load_order) { - std::cout << " # plugin load_order status\n"; - } else { - std::cout << " # plugin status\n"; - } - - for (const auto &p : info.plugins) { - std::cout << " " << std::right << std::setw(2) << p.index << " " << std::left << std::setw(30) << p.path; - - if (has_load_order) { - char order_buf[12]; - if (p.load_order >= 0) { - snprintf(order_buf, sizeof(order_buf), "%d", p.load_order); - } else { - snprintf(order_buf, sizeof(order_buf), "--"); - } - std::cout << " " << std::left << std::setw(11) << order_buf; - } - - std::cout << " " << p.status << '\n'; - } + _printer->write_output(response); } //------------------------------------------------------------------------------------------------------------------------------------ DirectRPCCommand::DirectRPCCommand(ts::Arguments *args) : CtrlCommand(args) diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index c0243c6ee24..4bf9a44b4f3 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -18,6 +18,8 @@ limitations under the License. */ +#include +#include #include #include #include @@ -700,3 +702,41 @@ ServerStatusPrinter::write_output(YAML::Node const &result) write_output_json(result["data"] ? result["data"] : result); } //------------------------------------------------------------------------------------------------------------------------------------- +void +PluginListPrinter::write_output(YAML::Node const &result) +{ + auto info = result.as(); + + std::cout << "source: " << info.source << '\n'; + + bool has_load_order = false; + for (const auto &p : info.plugins) { + if (p.load_order >= 0) { + has_load_order = true; + break; + } + } + + if (has_load_order) { + std::cout << " # plugin load_order status\n"; + } else { + std::cout << " # plugin status\n"; + } + + for (const auto &p : info.plugins) { + std::cout << " " << std::right << std::setw(2) << p.index << " " << std::left << std::setw(30) << p.path; + + if (has_load_order) { + char order_buf[12]; + if (p.load_order >= 0) { + snprintf(order_buf, sizeof(order_buf), "%d", p.load_order); + } else { + snprintf(order_buf, sizeof(order_buf), "--"); + } + std::cout << " " << std::left << std::setw(11) << order_buf; + } + + std::cout << " " << p.status << '\n'; + } +} +//------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/traffic_ctl/CtrlPrinters.h b/src/traffic_ctl/CtrlPrinters.h index dcae30837f8..684b6f80f6a 100644 --- a/src/traffic_ctl/CtrlPrinters.h +++ b/src/traffic_ctl/CtrlPrinters.h @@ -333,6 +333,14 @@ class ServerStatusPrinter : public BasePrinter ServerStatusPrinter(BasePrinter::Options opt) : BasePrinter(opt) {} }; //------------------------------------------------------------------------------------------------------------------------------------ +class PluginListPrinter : public BasePrinter +{ + void write_output(YAML::Node const &result) override; + +public: + PluginListPrinter(BasePrinter::Options opt) : BasePrinter(opt) {} +}; +//------------------------------------------------------------------------------------------------------------------------------------ /// In case a derived class needs to call derived class functions. Ugly but works. /// Note: CRTP may worth a try. From 2f2d9afc2c02d84b94f8c29f73c09fce57a0fd9c Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 3 Sep 2026 13:23:20 +0200 Subject: [PATCH 2/3] Add autests for traffic_ctl plugin list output Assert the payload through -f json rather than the text table. Column widths are a presentation detail, so pinning them down byte for byte only makes the test brittle against cosmetic changes. Text mode gets a smoke check that it still renders a table. The parse is the point. A gold file would have matched unparseable output indefinitely; the command itself had no autest before this. The empty plugin.config case gets its own test because a populated config never reaches it, and because TrafficCtl hardcodes its ATS process name, so two instances cannot share one file. Adds the plugin_config parameter the DSL was missing. The payload is asserted with validate_json_contains, naming the result node so the check also fails if it ever carries a sibling of data. --- .../traffic_ctl/traffic_ctl_json_null.test.py | 11 ++- .../traffic_ctl_plugin_empty.test.py | 45 +++++++++++ .../traffic_ctl_plugin_output.test.py | 75 +++++++++++++++++++ .../traffic_ctl/traffic_ctl_test_utils.py | 9 ++- 4 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py create mode 100644 tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py index 00a050f7791..134e7e1d652 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py @@ -72,14 +72,13 @@ ###### # plugin list -- `plugins` is empty when plugin.config loads nothing. # -# plugin list ignores the format flag today and prints a human table, so only -# the RPC path is assertable. Once plugin list honours -f json, add: -# traffic_ctl.plugin().list().as_json().validate_is_valid_json() +# This is the RPC path. The traffic_ctl -f json path is asserted in +# traffic_ctl_plugin_empty.test.py, which needs its own ATS because TrafficCtl +# hardcodes the process name. # # Assert the shape rather than mere parseability: `plugins` has to be `[]`, -# matching what the hostdb case above asserts for `partitions`. The field sits -# at result.data.plugins, which validate_json_contains cannot reach, so this -# compares the whole result, as the connection tracker cases in +# matching what the hostdb case above asserts for `partitions`. This compares +# the whole result, as the connection tracker cases in # traffic_ctl_server_output.test.py do. Before the fix the field emitted `~`, # which fails this comparison as surely as it fails a JSON parser. Nothing # forces plugin.config to be empty here, and nothing needs to: should a diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py new file mode 100644 index 00000000000..5fd436a0196 --- /dev/null +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_empty.test.py @@ -0,0 +1,45 @@ +# 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 = 'Test traffic_ctl plugin list with no plugins loaded.' + +Test.ContinueOnFail = True + +records_yaml = ''' + exec_thread: + autoconfig: + enabled: 0 + limit: 4 + ''' + +# No plugin_config, so the default empty plugin.config is used. This is the +# case a populated-config test never reaches, and the one that produced the +# unparseable `plugins: ~` before the json emitter was fixed. +traffic_ctl = Make_traffic_ctl(Test, records_yaml) + +# An empty list, not null. A client iterating this field should not have to +# special case a server with nothing loaded. Asserting the whole structure is +# what makes [] distinguishable from null here. +traffic_ctl.plugin().list().as_json().validate_json_contains(result={'data': {'source': 'plugin.config', 'plugins': []}}) + +traffic_ctl.plugin().list().validate_contains_all('source: plugin.config') diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py new file mode 100644 index 00000000000..f258dfeebe6 --- /dev/null +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_plugin_output.test.py @@ -0,0 +1,75 @@ +# 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 = 'Test traffic_ctl plugin list output in text and json format.' + +Test.ContinueOnFail = True + +Test.SkipUnless(Condition.PluginExists('xdebug.so'), Condition.PluginExists('stats_over_http.so')) + +records_yaml = ''' + exec_thread: + autoconfig: + enabled: 0 + limit: 4 + ''' + +# xdebug logs an error and does nothing when no feature is enabled. The +# reported path is argv[0] only, so the argument does not reach the table. +plugin_config = ['xdebug.so --enable=x-cache', 'stats_over_http.so'] + +traffic_ctl = Make_traffic_ctl(Test, records_yaml, plugin_config=plugin_config) + +# The payload is asserted through json, not through the table. Column widths +# are a presentation detail, so pinning them down only makes the test brittle +# against cosmetic changes. This checks the whole structure, so a renamed, +# added or dropped key fails too. Json mode emits the whole jsonrpc envelope, +# so the payload sits under result.data; naming result asserts it carries +# nothing else. +traffic_ctl.plugin().list().as_json().validate_json_contains( + result={ + 'data': + { + 'source': 'plugin.config', + 'plugins': + [ + { + 'path': 'xdebug.so', + 'enabled': 'true', + 'status': 'loaded', + 'index': '1' + }, + { + 'path': 'stats_over_http.so', + 'enabled': 'true', + 'status': 'loaded', + 'index': '2' + }, + ], + }, + }) + +# Text mode only needs to still render a table. This is a smoke check that the +# formatter survived being moved into PluginListPrinter; it deliberately does +# not assert the column layout. +traffic_ctl.plugin().list().validate_contains_all('source: plugin.config', 'xdebug.so', 'stats_over_http.so', 'loaded') diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 86e7e329daa..66cb82e2619 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -646,7 +646,7 @@ class TrafficCtl(Config, Server): Every time a config() is called, a new test is created. """ - def __init__(self, test, records_yaml=None, retcode=0): + def __init__(self, test, records_yaml=None, retcode=0, plugin_config=None): self._testNumber = 0 self._current_test_number = self._testNumber self._retcode = retcode @@ -654,6 +654,9 @@ def __init__(self, test, records_yaml=None, retcode=0): self._ts = self._Test.MakeATSProcess(f"ts_{self._testNumber}") if records_yaml != None: self._ts.Disk.records_config.update(records_yaml) + if plugin_config != None: + for line in plugin_config: + self._ts.Disk.plugin_config.AddLine(line) self._tests = [] def __get_index(self): @@ -695,6 +698,6 @@ def plugin(self): return Plugin(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) -def Make_traffic_ctl(test, records_yaml=None, retcode=0): - tctl = TrafficCtl(test, records_yaml, retcode) +def Make_traffic_ctl(test, records_yaml=None, retcode=0, plugin_config=None): + tctl = TrafficCtl(test, records_yaml, retcode, plugin_config) return tctl From 08e02421de4bcc097116ecd9ad5c36acda328c08 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 22 Sep 2026 10:48:36 +0200 Subject: [PATCH 3/3] traffic_ctl: render JSON field mismatches key-sorted This is the first caller to pass a nested expectation, and a nested value compared as a single repr() pair on one line, with the actual in emission order and the expected in source order, meant eyeballing two long inline dicts to find the differing key. Serialise both sides the same way, key-sorted, one per line. --- .../traffic_ctl/traffic_ctl_test_utils.py | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 66cb82e2619..8406285ff81 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -106,6 +106,24 @@ def _check_is_valid_json(path): return (True, desc, "Output parses as JSON") +def _render_json_value(value): + """Render a value for a failure message, key-sorted when it is a structure. + + A nested expectation compared as one `repr()` puts both structures on a + single line, in source order on one side and emission order on the other, + which is unreadable for anything larger than a scalar. Serialising both + sides the same way, key-sorted, makes the differing key findable. + + `default` catches a value json cannot serialise and non-string dict keys + raise regardless, so the call is guarded: autest treats an exception from + a tester callback as fatal, see the note in `_read_stdout`. + """ + try: + return json.dumps(value, sort_keys=True, default=repr) + except (TypeError, ValueError): + return repr(value) + + def _check_json_fields(path, expected): """Tester callback: every expected field must match its value in the parsed output. @@ -145,13 +163,15 @@ def _check_json_fields(path, expected): failed = [] for key, want in expected.items(): if key not in doc: - failed.append(f"{key} is missing (expected {want!r})") + failed.append(f"{key} is missing\n expected: {_render_json_value(want)}") continue actual = doc[key] if actual != want: - failed.append(f"{key} = {actual!r} (expected {want!r})") + failed.append( + f"{key} does not match\n actual : {_render_json_value(actual)}\n" + f" expected: {_render_json_value(want)}") if failed: - return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") + return (False, desc, "FAIL:\n" + "\n".join(failed) + f"\nOutput was:\n{raw}") return (True, desc, "All expected fields matched") @@ -230,9 +250,10 @@ 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. - Every mismatch is reported as "field_name = actual_value (expected expected_value)", - followed by the raw output. + Validate JSON output contains specific field:value pairs. Only checks specified fields, + each compared whole, so naming a field whose value is a structure asserts that subtree + exactly. Every mismatch is reported with the actual and expected values on their own + lines, key-sorted, 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