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