Copilot commented on code in PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749#discussion_r4147835806
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
Review Comment:
The PR description/title emphasize disabling *time-based* log rolling, but
setting `proxy.config.log.rolling_enabled` to 0 disables log rolling entirely
(not just midnight-based rolling). If the intent is truly to disable all
rolling in AuTests, please update the wording (including parameter/docstring)
to remove ambiguity. If the intent is only to stop time-based rolling while
keeping other rolling behaviors, consider leaving `rolling_enabled` on and
instead adjusting the time-based controls (e.g., interval/offset) so midnight
doesn’t trigger a rename.
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
"""Create a traffic server process.
:param block_for_debug: if True, causes traffic_server to run with the
--block option enabled, and effectively disables timeouts that could be
triggered by running traffic_server under a debugger. In the debugger,
`set cmd_block = 0`, set any desired break points, then `c` to continue
to let the test proceed.
+
+ :param disable_log_rolling: if True (the default), turn off time-based log
+ rolling, so that a test running across 00:00 UTC does not have its logs
+ renamed before its content checks read them. Pass False to leave the
Review Comment:
The docstring and inline comment describe the behavior as turning off
*time-based* rolling, but the implementation disables rolling globally via
`proxy.config.log.rolling_enabled`. Please align the documentation with the
actual behavior (either clarify that all rolling is disabled, or adjust
implementation to only disable the time-based aspect).
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
Review Comment:
Given the documentation focuses on time-based rolling, `disable_log_rolling`
is potentially ambiguous (it reads like it disables all rolling, which is what
the current implementation does). Consider renaming to a more explicit
parameter name (e.g., `disable_all_log_rolling` if that’s intended, or
`disable_time_based_log_rolling` if the intent is only time-based behavior) to
reduce misuse and keep call sites self-documenting.
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +400,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect
the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms':
20})
+ # The default time-based log rolling renames every log at 00:00 UTC. A
test that is running
+ # across midnight then checks the fresh, empty file and fails. Tests that
exercise rolling
+ # set this themselves, which overrides the default here.
Review Comment:
The docstring and inline comment describe the behavior as turning off
*time-based* rolling, but the implementation disables rolling globally via
`proxy.config.log.rolling_enabled`. Please align the documentation with the
actual behavior (either clarify that all rolling is disabled, or adjust
implementation to only disable the time-based aspect).
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +400,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect
the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms':
20})
+ # The default time-based log rolling renames every log at 00:00 UTC. A
test that is running
+ # across midnight then checks the fresh, empty file and fails. Tests that
exercise rolling
+ # set this themselves, which overrides the default here.
+ if disable_log_rolling:
+ p.Disk.records_config.update({'proxy.config.log.rolling_enabled': 0})
Review Comment:
The PR description/title emphasize disabling *time-based* log rolling, but
setting `proxy.config.log.rolling_enabled` to 0 disables log rolling entirely
(not just midnight-based rolling). If the intent is truly to disable all
rolling in AuTests, please update the wording (including parameter/docstring)
to remove ambiguity. If the intent is only to stop time-based rolling while
keeping other rolling behaviors, consider leaving `rolling_enabled` on and
instead adjusting the time-based controls (e.g., interval/offset) so midnight
doesn’t trigger a rename.
--
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]
