This document collects common comments made during reviews of Tast tests.
Tast code should also follow Go's established best practices as described in Effective Go and Go Code Review Comments.
Go Style (internal document similar to public style guides) is also a good read and source of style suggestions.
In general you should not parse the result of CombinedOutput
. Its result interleaves stdout and stderr, so parsing it is very likely flaky.
If the message you are concerned with is written to stdout, use Output
instead. In the case of stderr, capture it explicitly with Run
.
// GOOD out, err := testexec.CommandContext(...).Output() if regexp.Match(out, "...") { ... }
// GOOD cmd := testexec.CommandContext(...) var stderr bytes.Buffer cmd.Stderr = &stderr if err := cmd.Run(...); err != nil { ... } out := stderr.Bytes() if regexp.Match(out, "...") { ... }
// BAD out, err := testexec.CommandContext(...).CombinedOutput() if regexp.Match(out, "...") { ... }
After calling functions to create a new context.Context with a new deadline (e.g. ctxutil.Shorten
, context.WithTimeout
etc.), always call the cancel function with a defer statement. In many cases those functions start a new goroutine associated with the new context, and it is released only on cancellation or expiration of the context. Thus failing to cancel the context may lead to resource leaks.
// GOOD ctx, cancel := ctxutil.Shorten(ctx, 5*time.Second) defer cancel()
// BAD ctx, _ = ctxutil.Shorten(ctx, 5*time.Second)
This article describes how a similar bug caused massive production issues at Square.
context.Context
carries the deadline of a function call. Functions that may block should take context.Context
as an argument and honor its deadline.
// GOOD func httpGet(ctx context.Context, url string) ([]byte, error) { ... }
// BAD func httpGet(url string) ([]byte, error) { ... }
Whenever possible, use fixtures rather than calling setup functions by yourself. Fixtures speeds up testing when multiple tests sharing the same fixtures are run at a time (e.g. in the commit queue).
// GOOD func init() { testing.AddTest(&testing.Test{ Func: Example, Fixture: "chromeLoggedIn", ... }) }
// BAD func Example(ctx context.Context, s *testing.State) { cr, err := chrome.New(ctx) if err != nil { s.Fatal("Failed to start Chrome: ", err) } ... }
See also a section in the Writing Tests document.
Using the os.Chdir
function in Tast tests can make tests flakier because it creates a potential race condition as the current working direstory (CWD) is shared across the running process. If commands need to be run in a specific directory, consider using exec.Command
and updating the Dir field to the directory the command needs to run in.
// GOOD func Example(ctx context.Context, s *testing.State) { cmd := exec.Command("ls") cmd.Dir = "tmp" err := cmd.Run() if err != nil { s.Fatal("Failed to list in folder tmp: ", err) } ... }
// BAD func Example(ctx context.Context, s *testing.State) { err := os.Chdir('tmp') if err != nil { s.Fatal("Failed to switch directory: ", err) } cmd := exec.Command("ls") err := cmd.Run() if err != nil { s.Fatal("Failed to list in folder tmp: ", err) } ... }
Sleeping without polling for a condition is discouraged, since it makes tests flakier (when the sleep duration isn't long enough) or slower (when the duration is too long).
The testing.Poll
function makes it easier to honor timeouts while polling for a condition:
// GOOD startServer() if err := testing.Poll(ctx, func (ctx context.Context) error { return checkServerReady() }, &testing.PollOptions{Timeout: 10 * time.Second}); err != nil { return errors.Wrap(err, "server failed to start") }
// BAD startServer() testing.Sleep(ctx, 10*time.Second) // hope that the server starts in 10 seconds
If you really need to sleep for a fixed amount of time, use testing.Sleep
to honor the context timeout.
testing.State
implements a lot of methods allowing tests to access all the information and utilities they may use to perform testing. Since it contains many things, passing it to a function as an argument makes it difficult to tell what are inputs and outputs of the function from its signature. Also, such a function cannot be called from gRPC services.
For this reason, tast-lint
forbids use of testing.State
in support library packages. Other packages, such as test main functions and test subpackages, can still use testing.State
, but think carefully if you really need it.
In many cases you can avoid passing testing.State
to a function:
error
value, and call testing.State.Error
(or its family) at the highest possible level in the call stack.context.Context
so it can call testing.ContextLog
.testing.State.OutDir
. Alternatively you can pass a context.Context
and call testing.ContextOutDir
.Avoid skipping tests or subtests by checking device capabilities at runtime. Instead specify software/hardware dependencies to declare software features and/or hardware capabilities your test depends on.
// GOOD func init() { testing.AddTest(&testing.Test{ Func: CheckCamera, SoftwareDeps: []string{"camera_720p", "chrome"}, ... }) }
// BAD func CheckCamera(ctx context.Context, s *testing.State) { if !supports720PCamera() { s.Log("Skipping test; device unsupported") return } ... }
See also a section in the Writing Tests document.