-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
util: improve log.Scope runtime output handling #51409
Comments
Hi @tbg, please add a C-ategory label to your issue. Check out the label system docs. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
This has been investigated in practice. We have two separate problems to solve and two separate approaches, with different trade-offs. Here is a summary.
Here are the two solutions that have been researched:
In light of how #51410 fares poorly in problem 2, and how #51499 addresses both in a consistent manner, I recommend selecting the latter approach. |
Already mentioned in DM, but I didn't see a reason why this should be true, and in a local experiment did not see this, so I wonder what's going on there. I'll take another look. |
Yeah I don't know, this seems to work just fine. With this test: func TestFoo(t *testing.T) {
go func() {
panic("big boom!")
}()
time.Sleep(time.Second)
} I get via
the following
|
fixed by #52200 |
Consider the following test which panics off the main goroutine (it also does other things, but just ignore that):
The output is as follows:
First of all I'm unsure whether "i'm on stdout" is supposed to be visible here. Why didn't it get redirected to the logs?
Secondly and more importantly, the panic got stashed into
$TMPDIR/logTestFoo927366034/stoptest-stderr.log
and does not have appropriate visibility (to unskilled investigators).The Go test harness will not mark such tests as failed (rightly so, as multiple tests could have been running in parallel, so it can't blame any of them, though it would be desirable if in the absence of parallelism it did emit a FAIL line for the one running test which it does not).
Since the panic is off the main goroutine (as it often is in practice when we start test clusters, etc) the
Scope
can't usedefer
to handle it (in fact, the defer will never run in this case).My best suggestion is to change
log.Scope
to keepos.Stderr
untouched (or perhaps change it into a TeeWriter that goes to a log and actual stderr) and only redirect log output to the file.s
However, even with this, since no test is marked as failed, the output is not readily available, though at least it will be in the build log. Adding to the complications, the build log is not very useful since we're using JSON output, which TeamCity makes hard to see. I would still suggest we try this approach to figure out the next step(s).
The text was updated successfully, but these errors were encountered: