Traffic ctl plugin list format

src/traffic_ctl/CtrlCommands.cc

@@ -26,7 +26,6 @@
 #include <fstream>
 #include <unordered_map>
 #include <chrono>
-#include <iomanip>
 #include <utility>
 #include <thread>
 #include <unistd.h>
@@ -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<GenericPrinter>(printOpts);
   } else if (get_parsed_arguments()->get(LIST_STR)) {
     _invoked_func = [&]() { plugin_list(); };
+    _printer      = std::make_unique<PluginListPrinter>(printOpts);
+  } else {
+    _printer = std::make_unique<GenericPrinter>(printOpts);
   }
-  _printer = std::make_unique<GenericPrinter>(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<PluginListResponse>();
-
-  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)

src/traffic_ctl/CtrlPrinters.cc

@@ -18,6 +18,8 @@
   limitations under the License.
 */
 
+#include <cstdio>
+#include <iomanip>
 #include <iostream>
 #include <unordered_map>
 #include <string_view>
@@ -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<PluginListResponse>();
+
+  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';
+  }
+}
+//-------------------------------------------------------------------------------------------------------------------------------------

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.

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

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')

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')

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
@@ -646,14 +667,17 @@ 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
         self._Test = test
         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 +719,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