feat: capture cli logs in tests #10669
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
31c8047 to
fcd5e39
Compare
bea860d to
ca45c99
Compare
| @@ -44,17 +44,17 @@ func TestWorkspaceAgent(t *testing.T) { | |||
| "--log-dir", logDir, | |||
| ) | |||
|
|
|||
| pty := ptytest.New(t).Attach(inv) | |||
There was a problem hiding this comment.
attaching a PTY to agent invocation results in logging twice to the test log: once via our logger and a second time via stderr. I modified the test to not need the PTY at all.
cli/agent_test.go
Outdated
| "cdr.dev/slog" | ||
| "cdr.dev/slog/sloggers/slogtest" | ||
|
|
||
| "github.com/coder/coder/v2/testutil" |
There was a problem hiding this comment.
Import order is messed up here. Usually due to editor inserting on autocomplete but in the wrong place and formatter not being strict to repair it :/.
| @@ -170,6 +172,7 @@ func (c *Cmd) Invoke(args ...string) *Invocation { | |||
| Stdout: io.Discard, | |||
| Stderr: io.Discard, | |||
| Stdin: strings.NewReader(""), | |||
| Logger: slog.Make(), | |||
There was a problem hiding this comment.
At least for tests, this could be a named logger based on the command being run? Might help when there are multiple commands being run.
There was a problem hiding this comment.
Right now, it's just called "cli" for any Invocations created by clitest.New(). You can override it in any complex test by setting inv.Logger before starting the command.
cli/cliutil/sink.go
Outdated
| d.Lock() | ||
| defer d.Unlock() | ||
| if d.closed { | ||
| return len(p), nil |
There was a problem hiding this comment.
Does slog print errors if we return io.EOF here? If yes and since this writer has no way to inform routines using it to stop, should we rename it something slog specific since we're essentially working around the fact that slog doesn't let you close it or close/detach sinks.
Also, if we need this at all, it means we're leaving goroutines behind when exciting, right? Often it's not a problem but it could mean we're not shutting down things in the proper order. This would essentially hide that.
There was a problem hiding this comment.
Does slog print errors if we return io.EOF here?
the Logger does not, but some of the Sinks call println if they get an error writing, which I'd like to avoid.
If yes and since this writer has no way to inform routines using it to stop, should we rename it something slog specific since we're essentially working around the fact that slog doesn't let you close it or close/detach sinks.
I did mention slog in the comment on the constructor, but I'm open to a name change if you have a suggestion.
Also, if we need this at all, it means we're leaving goroutines behind when exciting, right? Often it's not a problem but it could mean we're not shutting down things in the proper order. This would essentially hide that.
Yes, many of our components just use a Context expiring to know when to shut down, and others that have an explicit Close() don't wait for all the goroutines to exit before returning. These components are still "correct" in the sense that the goroutines do exit eventually---they're not leaks, just shut down is async with no way to wait for it.
We could mandate that everything has a Close() and it doesn't return until every goroutine is shut down, but that's pretty heavy handed and a lot of work.
If there are actual leaked goroutines, then we have goleak to detect those in tests. It's not worth trying to use the logger to police that.
ca45c99 to
ac451ac
Compare
Merge activity
|
Adds a Logger to cli Invocation and standardizes CLI commands to use it. clitest creates a test logger by default so that CLI command logs are captured in the test logs. CLI commands that do their own log configuration are modified to add sinks to the existing logger, rather than create a new one. This ensures we still capture logs in CLI tests.
Adds a Logger to cli Invocation and standardizes CLI commands to use it. clitest creates a test logger by default so that CLI command logs are captured in the test logs.
CLI commands that do their own log configuration are modified to add sinks to the existing logger, rather than create a new one. This ensures we still capture logs in CLI tests.