github.com/nya3jp/tast@v0.0.0-20230601000426-85c8e4d83a9b/docs/code_review_comments.md (about) 1 # Tast: Code Review Comments (go/tast-code-review-comments) 2 3 This document collects common comments made during reviews of Tast tests. 4 5 Tast code should also follow Go's established best practices as described in 6 [Effective Go] and [Go Code Review Comments]. 7 [Go Style] (internal document similar to public style guides) is also a good 8 read and source of style suggestions. 9 10 [Effective Go]: https://golang.org/doc/effective_go.html 11 [Go Code Review Comments]: https://github.com/golang/go/wiki/CodeReviewComments 12 [Go Style]: http://go/go-style 13 14 [TOC] 15 16 17 ## CombinedOutput 18 19 In general you should not parse the result of [`CombinedOutput`]. 20 Its result interleaves stdout and stderr, so parsing it is very likely flaky. 21 22 If the message you are concerned with is written to stdout, use [`Output`] instead. 23 In the case of stderr, capture it explicitly with [`Run`]. 24 25 ```go 26 // GOOD 27 out, err := testexec.CommandContext(...).Output() 28 if regexp.Match(out, "...") { ... } 29 ``` 30 31 ```go 32 // GOOD 33 cmd := testexec.CommandContext(...) 34 var stderr bytes.Buffer 35 cmd.Stderr = &stderr 36 if err := cmd.Run(...); err != nil { ... } 37 out := stderr.Bytes() 38 if regexp.Match(out, "...") { ... } 39 ``` 40 41 ```go 42 // BAD 43 out, err := testexec.CommandContext(...).CombinedOutput() 44 if regexp.Match(out, "...") { ... } 45 ``` 46 47 [`CombinedOutput`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast-tests.git/src/go.chromium.org/tast-tests/cros/common/testexec#Cmd.CombinedOutput 48 [`Output`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast-tests.git/src/go.chromium.org/tast-tests/cros/common/testexec#Cmd.Output 49 [`Run`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast-tests.git/src/go.chromium.org/tast-tests/cros/common/testexec#Cmd.Run 50 51 52 ## Context cancellation 53 54 After calling functions to create a new context.Context with a new deadline 55 (e.g. [`ctxutil.Shorten`], [`context.WithTimeout`] etc.), always call the cancel 56 function with a defer statement. In many cases those functions start a new 57 goroutine associated with the new context, and it is released only on 58 cancellation or expiration of the context. Thus failing to cancel the context 59 may lead to resource leaks. 60 61 ```go 62 // GOOD 63 ctx, cancel := ctxutil.Shorten(ctx, 5*time.Second) 64 defer cancel() 65 ``` 66 67 ```go 68 // BAD 69 ctx, _ = ctxutil.Shorten(ctx, 5*time.Second) 70 ``` 71 72 [This article](https://developer.squareup.com/blog/always-be-closing/) 73 describes how a similar bug caused massive production issues at Square. 74 75 [`ctxutil.Shorten`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/ctxutil#Shorten 76 [`context.WithTimeout`]: https://godoc.org/context#WithTimeout 77 78 79 ## Context timeout 80 81 [`context.Context`] carries the deadline of a function call. Functions that may 82 block should take [`context.Context`] as an argument and honor its deadline. 83 84 ```go 85 // GOOD 86 func httpGet(ctx context.Context, url string) ([]byte, error) { ... } 87 ``` 88 89 ```go 90 // BAD 91 func httpGet(url string) ([]byte, error) { ... } 92 ``` 93 94 [`context.Context`]: https://godoc.org/context 95 96 97 ## Fixtures 98 99 Whenever possible, use [fixtures] rather than calling setup functions by 100 yourself. Fixtures speeds up testing when multiple tests sharing the same 101 fixtures are run at a time (e.g. in the commit queue). 102 103 ```go 104 // GOOD 105 func init() { 106 testing.AddTest(&testing.Test{ 107 Func: Example, 108 Fixture: "chromeLoggedIn", 109 ... 110 }) 111 } 112 ``` 113 114 ```go 115 // BAD 116 func Example(ctx context.Context, s *testing.State) { 117 cr, err := chrome.New(ctx) 118 if err != nil { 119 s.Fatal("Failed to start Chrome: ", err) 120 } 121 ... 122 } 123 ``` 124 125 See also [a section in the Writing Tests document](writing_tests.md#Fixtures). 126 127 [fixtures]: writing_tests.md#Fixtures 128 129 130 ## os.Chdir 131 132 Using the [`os.Chdir`] function in Tast tests can make tests flakier 133 because it creates a potential race condition as the current working 134 direstory (CWD) is shared across the running process. If commands need to 135 be run in a specific directory, consider using [`exec.Command`] and 136 updating the Dir field to the directory the command needs to run in. 137 138 ```go 139 // GOOD 140 func Example(ctx context.Context, s *testing.State) { 141 cmd := exec.Command("ls") 142 cmd.Dir = "tmp" 143 err := cmd.Run() 144 145 if err != nil { 146 s.Fatal("Failed to list in folder tmp: ", err) 147 } 148 ... 149 } 150 151 152 ``` 153 154 ```go 155 // BAD 156 func Example(ctx context.Context, s *testing.State) { 157 err := os.Chdir('tmp') 158 if err != nil { 159 s.Fatal("Failed to switch directory: ", err) 160 } 161 cmd := exec.Command("ls") 162 err := cmd.Run() 163 if err != nil { 164 s.Fatal("Failed to list in folder tmp: ", err) 165 } 166 ... 167 } 168 ``` 169 170 [`os.Chdir`]: https://pkg.go.dev/os#Chdir 171 [`exec.Command`]: https://pkg.go.dev/os/exec#Command 172 173 ## Sleep 174 175 Sleeping without polling for a condition is discouraged, since it makes tests 176 flakier (when the sleep duration isn't long enough) or slower (when the duration 177 is too long). 178 179 The [`testing.Poll`] function makes it easier to honor timeouts while polling 180 for a condition: 181 182 ```go 183 // GOOD 184 startServer() 185 if err := testing.Poll(ctx, func (ctx context.Context) error { 186 return checkServerReady() 187 }, &testing.PollOptions{Timeout: 10 * time.Second}); err != nil { 188 return errors.Wrap(err, "server failed to start") 189 } 190 ``` 191 192 ```go 193 // BAD 194 startServer() 195 testing.Sleep(ctx, 10*time.Second) // hope that the server starts in 10 seconds 196 ``` 197 198 If you really need to sleep for a fixed amount of time, use [`testing.Sleep`] to 199 honor the context timeout. 200 201 [`testing.Poll`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#Poll 202 [`testing.PollBreak`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#PollBreak 203 [`testing.Sleep`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#Sleep 204 205 206 ## State 207 208 [`testing.State`] implements a lot of methods allowing tests to access all the 209 information and utilities they may use to perform testing. Since it contains 210 many things, passing it to a function as an argument makes it difficult to 211 tell what are inputs and outputs of the function from its signature. Also, 212 such a function cannot be called from [gRPC services]. 213 214 For this reason, `tast-lint` forbids use of [`testing.State`] in support 215 library packages. Other packages, such as test main functions and test 216 subpackages, can still use [`testing.State`], but think carefully if you really 217 need it. 218 219 In many cases you can avoid passing [`testing.State`] to a function: 220 221 * If a function needs to report an error, just return an `error` value, 222 and call [`testing.State.Error`] (or its family) at the highest possible 223 level in the call stack. 224 * If a function needs to log its progress, pass a [`context.Context`] so it 225 can call [`testing.ContextLog`]. 226 * If a function needs to write to the output directory, just pass the path 227 returned by [`testing.State.OutDir`]. Alternatively you can pass a 228 [`context.Context`] and call [`testing.ContextOutDir`]. 229 230 [gRPC services]: https://chromium.googlesource.com/chromiumos/platform/tast/+/HEAD/docs/writing_tests.md#Remote-procedure-calls-with-gRPC 231 [`testing.State`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#State 232 [`testing.State.Error`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#State.Error 233 [`context.Context`]: https://godoc.org/context 234 [`testing.ContextLog`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#ContextLog 235 [`testing.State.OutDir`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#State.OutDir 236 [`testing.ContextOutDir`]: https://godoc.org/chromium.googlesource.com/chromiumos/platform/tast.git/src/go.chromium.org/tast/core/testing#ContextOutDir 237 [the allowlist in tast-lint]: https://chromium.googlesource.com/chromiumos/platform/tast/+/refs/heads/main/src/go.chromium.org/tast/core/cmd/tast-lint/check/disallow_testingstate.go 238 239 240 ## Test dependencies 241 242 Avoid skipping tests or subtests by checking device capabilities at runtime. 243 Instead specify [software/hardware dependencies] to declare software features 244 and/or hardware capabilities your test depends on. 245 246 ```go 247 // GOOD 248 func init() { 249 testing.AddTest(&testing.Test{ 250 Func: CheckCamera, 251 SoftwareDeps: []string{"camera_720p", "chrome"}, 252 ... 253 }) 254 } 255 ``` 256 257 ```go 258 // BAD 259 func CheckCamera(ctx context.Context, s *testing.State) { 260 if !supports720PCamera() { 261 s.Log("Skipping test; device unsupported") 262 return 263 } 264 ... 265 } 266 ``` 267 268 See also [a section in the Writing Tests document](writing_tests.md#device-dependencies). 269 270 [software/hardware dependencies]: test_dependencies.md