Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign uproachtest: fix stdout output #31405
Conversation
andreimatei
assigned
benesch
Oct 15, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
This change is |
petermattis
requested changes
Oct 16, 2018
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/cmd/roachtest/test.go, line 872 at r1 (raw file):
artifactsDir: t.ArtifactsDir(), localCluster: local, teeToStdout: parallelism == 1,
Isn't parallelism still the global here? Even if this is somehow correct, I suggest getting rid of the global and tying the flag initialization to a local variable in main(). That will prevent accidental use of the global.
benesch
removed their assignment
Oct 16, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Deferring this review to @petermattis. |
andreimatei
reviewed
Oct 16, 2018
Reviewable status:
pkg/cmd/roachtest/test.go, line 872 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Isn't
parallelismstill the global here? Even if this is somehow correct, I suggest getting rid of the global and tying the flag initialization to a local variable inmain(). That will prevent accidental use of the global.
yes, I had failed.
I've done what you said; good suggestion. I wanted to truly get rid of some of these globals, but I failed to read properly how we use Cobra and thus didn't see the obvious.
petermattis
approved these changes
Oct 16, 2018
Reviewable status:
bot
pushed a commit
that referenced
this pull request
Oct 16, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
craig
bot
commented
Oct 16, 2018
Build succeeded |
andreimatei commentedOct 15, 2018
roachtest's intention is that if there's no parallelism is requested,
logs are teed to stdout/stderr in addition to the test.log file. No
parallelism is specified explicitly by passing -p 1, or implicitly by
running a single test.
This patch fixes a bug where the "implicit" part was no longer behaving
as expected wrt the logs. The bug was introduced by a previous patch
that tried to reduce the scope of the "parallelism" global, but missed
the use in rootLogger(); the inferring of parallelism=1 was no longer
modifying the global. This patch kills the global for good.
Release note: None