-
Notifications
You must be signed in to change notification settings - Fork 887
ci: Improve gotestsum failure detection, prevent early exit #5420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9d8ea2f
to
bce52ce
Compare
591c69e
to
d0a3e50
Compare
ret=$? | ||
if ((ret)); then | ||
# Eternalize test timeout logs because "re-run failed" erases | ||
# artifacts and gotestsum doesn't always capture it: | ||
# https://github.com/gotestyourself/gotestsum/issues/292 | ||
echo "Checking gotestsum.json for panic trace:" | ||
grep -A 999999 'panic: test timed out' gotestsum.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review: This was pretty loud (so many logs after timeout), so I decided to only capture the stack trace, and while I was at it, only when the criteria is met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR fixes early exit (missing
set +e
) which prevented theif
statement from being executed.Since the log was too noisy, I applied filtering and edge case detection.
Example output after this PR: https://github.com/coder/coder/actions/runs/3697831254/jobs/6273476923
PS. I never realized, but it seems setting
-timeout=Xm
like we do applies on a per-package basis, not the entire run ofgo test
. As a result, the entire run can be much longer it seems 🤔 (or am I misunderstanding something?)