bneradt commented on PR #13021: URL: https://github.com/apache/trafficserver/pull/13021#issuecomment-4143864694
> The shell-script approach is a big improvement over the old Process chain. Much easier to follow the sequencing, and the explicit polling loops with 60-second timeouts should handle the ASAN/CI slowness that was causing the flakiness. > > Two things I noticed: > > 1. **Missing `StillRunningAfter`** -- The old test had `tr1.StillRunningAfter = diags_test.ts` (and the server) to verify ATS survived the signal. Neither `add_system_log_test` nor `add_configured_log_test` sets this. Without it, if ATS crashes during SIGUSR2 handling, the test could still pass as long as the shell script exits 0 (the shell script's `wait_for_contains` would succeed on files written before the crash). Might be worth adding back. Actually, with this change, we were able to associate the ATS instance with the more specific TestRun itself rather than the global Test namespace. That's a good thing since it is a cleaner solution - it's easier to think through the processes if they are all neatly associated with their own TestRun. Therefore the TS instance will not be "StillRunningAfter" - it intentionally is cleaned up when the TestRun completes. That said, any crash would manifest itself as a non-zero return in TS and will be seen via that mechanism. > 2. **Typo in commit message** -- "shel helper script" should be "shell helper script" (appears twice). Fixed. -- 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]
