Dynamic PTS for test runs#129
Conversation
f3735dc to
4c024b0
Compare
| tmpdir=$(mktemp -d) | ||
|
|
||
| cleanup() { | ||
| pkill -P $$ 2>/dev/null || true |
There was a problem hiding this comment.
pkill -P $$ kills only direct children of the current shell. If diagslave or socat spawns child processes, those will be orphaned. Consider using a process group kill (kill -- -$$) or tracking PIDs explicitly.
The || true suppresses all errors, including unexpected ones, which may hide issues in CI.
There was a problem hiding this comment.
They don't spawn any but still good point.
| ;; | ||
| rtu) | ||
| socat -d -d pty,raw,echo=0,link="$tmpdir/pty0" pty,raw,echo=0,link="$tmpdir/pty1" & | ||
| sleep 0.5 |
There was a problem hiding this comment.
sleep 0.5 is a race condition
The sleep 0.5 waiting for socat to create the PTY symlinks is a classic timing hack. On a loaded CI machine this can be too short. A more robust approach would be to poll until the symlink appears, e.g. until [ -e "$tmpdir/pty0" ]; do sleep 0.05; done with a timeout guard.
Also in line 30
| diagslave -m rtu "$tmpdir/pty1" & | ||
| go test -run "$TEST_FILTER" -v . | ||
| ;; | ||
| ascii) |
There was a problem hiding this comment.
rtu and ascii do the same thing except for the -m flag. We could avoid the copy-paste by using one shared branch/helper and just changing the MODE passed to diagslave.
| MODE=$1 | ||
| TEST_FILTER=$2 |
There was a problem hiding this comment.
MODE and TEST_FILTER are used without much validation. MODE already falls through to the usage error if it’s missing, but an empty TEST_FILTER becomes -run "", which runs all tests. If that’s intended, maybe we should document it.
There was a problem hiding this comment.
set -e won’t catch failures from background processes. If diagslave or socat fails to start, the later go test may fail with a less clear error. It would be easier to debug if we check that the background process is actually running before continuing. Wdyt?
glidecli is not really necessary with model go. So that was cleaned up tooFixes: #128