bryancall commented on PR #12867: URL: https://github.com/apache/trafficserver/pull/12867#issuecomment-3867651490
## Review Feedback Response Addressed the automated review comments in commit 3e28d16350. Here's what was fixed and what was intentionally left as-is: ### Fixed (6 items) | # | Comment | Fix | |---|---------|-----| | 1 | `AUTEST_PORT_OFFSET` in `ports.py` should validate input | Added `try/except ValueError` with warning + clamped to `[0, 60000]` range | | 2 | `--ats-bin` is required even for `--list` mode which doesn't need it | Changed to `default=None` with manual validation — only required when actually running tests | | 3 | Verbose mode `Popen` uses blocking `for line in proc.stdout` with no timeout | Added `proc.wait(timeout=60)` with `TimeoutExpired` handling after stdout is exhausted (the line-by-line read itself completes when the process closes stdout) | | 4 | `load_serial_tests` docstring says "matches any test containing this" but code uses exact basename match | Updated docstring to accurately describe the `.stem` extraction behavior | | 5 | `current_test` and `test_start_line` are assigned but never used in `parse_autest_output` | Removed the unused variables | | 6 | Exception handlers (`except IOError: pass`, `except (json.JSONDecodeError, IOError): pass`) lack context | Added inline comments explaining why the exceptions are intentionally silenced | ### Not Fixed (2 items) | # | Comment | Reason | |---|---------|--------| | 1 | Suggestion to use `autest.sh` wrapper instead of `uv run autest` | `autest.sh` is the CMake-generated wrapper that calls `uv run autest` internally. `autest-parallel.py` runs independently of CMake and invokes `uv run autest` directly, which is the correct approach. | | 2 | Suggestion that `for line in proc.stdout` blocks indefinitely | This is expected behavior — the iterator completes when the subprocess closes its stdout pipe (i.e., when it exits). The 1-hour timeout on the non-verbose `subprocess.run` path protects against hangs there, and the new `proc.wait(timeout=60)` guards the post-read cleanup in verbose mode. | -- 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]
