Copilot commented on code in PR #13862:
URL: https://github.com/apache/trafficserver/pull/13862#discussion_r4326490327
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -140,7 +140,7 @@ def AddConfigReload(
data=None,
force=False,
timeout="30s",
- initial_wait=1.0,
+ initial_wait=None,
Review Comment:
Changing the helper default from an explicit value (`1.0`) to “omit the
flag” makes test behavior depend on `traffic_ctl`’s current default (and
changes automatically if that default changes later). If the goal is only to
avoid the `-w` short option conflict, consider keeping an explicit numeric
default (passed via `--initial-wait`) so test timing/latency remains stable and
independent of CLI defaults.
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -166,7 +166,7 @@ def AddConfigReload(
force: If True, adds --force.
timeout: Timeout duration string for --timeout (e.g. "30s", "1m").
Set to None to disable.
- initial_wait: Seconds before first poll (--initial-wait).
+ initial_wait: Seconds before first poll (--initial-wait). None leaves
traffic_ctl's own default.
Review Comment:
Changing the helper default from an explicit value (`1.0`) to “omit the
flag” makes test behavior depend on `traffic_ctl`’s current default (and
changes automatically if that default changes later). If the goal is only to
avoid the `-w` short option conflict, consider keeping an explicit numeric
default (passed via `--initial-wait`) so test timing/latency remains stable and
independent of CLI defaults.
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -199,7 +199,9 @@ def AddConfigReload(
tr.StillRunningAfter = ts
cmd = f"traffic_ctl config reload -m -t {_shell_quote(token)}"
- cmd += f" -w {initial_wait} -r {refresh_int}"
+ cmd += f" -r {refresh_int}"
+ if initial_wait is not None:
+ cmd += f" --initial-wait {initial_wait}"
Review Comment:
Changing the helper default from an explicit value (`1.0`) to “omit the
flag” makes test behavior depend on `traffic_ctl`’s current default (and
changes automatically if that default changes later). If the goal is only to
avoid the `-w` short option conflict, consider keeping an explicit numeric
default (passed via `--initial-wait`) so test timing/latency remains stable and
independent of CLI defaults.
##########
include/tscore/ArgParser.h:
##########
@@ -364,6 +394,14 @@ class ArgParser
// Enable test mode - makes do_exit() throw instead of calling exit() for
unit testing
static void set_test_mode(bool test = true);
+ /** Clear the parser-wide state held at file scope in ArgParser.cc.
+
+ Test-only. A program builds one parser and never reuses the state, but a
unit test binary
+ runs many parsers in sequence, and set_default_command() or
Command::set_default() would
+ otherwise leak default_command into every later test.
+ */
+ static void reset_global_state();
Review Comment:
`reset_global_state()` is described as “Test-only” but is added to the
public API in an installed header. If this is only intended for unit tests,
consider restricting visibility (e.g., compile it under a test-only macro, move
it to a non-installed header, or make it protected/private and expose it to
tests via a friend/test accessor) to avoid encouraging production code to
depend on test-only global-state controls.
##########
src/tscore/unit_tests/test_ArgParser.cc:
##########
@@ -719,3 +723,775 @@ TEST_CASE("The variable only applies when every
occurrence of a repeated option
REQUIRE(parsed.get("directive").size() == 0);
}
}
+
+namespace
+{
+// Parser-wide state lives at file scope in ArgParser.cc and outlives any one
parser: a test
+// that reaches the help path leaves usage_return_code at 0 for every test
after it, and a test
+// that sets a default command leaves that too. Clear it on the way in and on
the way out.
+class ArgParserStateGuard
+{
+public:
+ ArgParserStateGuard() { reset(); }
+ ~ArgParserStateGuard() { reset(); }
+ static void
+ reset()
+ {
+ Access::reset_global_state();
+ }
+
+private:
+ struct Access : ts::ArgParser {
+ using ts::ArgParser::reset_global_state;
+ };
+};
+
+// Keeps the help and usage text that a parser prints out of the test output,
and lets it be
+// checked. std::cout is put back however the scope ends.
+struct CoutCapture {
+ std::ostringstream buf;
+ std::streambuf *old = std::cout.rdbuf(buf.rdbuf());
+ ~CoutCapture() { std::cout.rdbuf(old); }
+};
Review Comment:
`CoutCapture` is implicitly copyable/movable, which can lead to
double-restoration of `std::cout`’s rdbuf if it’s ever copied (even
accidentally). Making it non-copyable (and ideally non-movable) would harden
the test helper against subtle misuse while keeping the same call sites.
--
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]
