github.com/nya3jp/tast@v0.0.0-20230601000426-85c8e4d83a9b/docs/code_reviews.md (about)

     1  # Getting Code Reviews for Tast Tests (go/tast-reviews)
     2  
     3  ## Summary
     4  
     5  To submit a Tast test change, please follow the following steps.
     6  
     7  Firstly, do a **self-review** by going through the [self-review checklist]
     8  below. This will save your time by avoiding typical code review comments in the
     9  later step.
    10  
    11  Once the self-review is done, send your change to the following two sets of
    12  reviewers:
    13  
    14  1.  **Test owners**. If your change modifies an existing test, send it to one of
    15      the owners of the test (those listed in the `Contacts` field of the test
    16      declaration, excluding yourself). If your change introduces a new test, send
    17      it to your team member(s) who will co-own the test. Getting reviews from the
    18      test owners makes sure the change is good from the perspective of feature
    19      experts [(details)](#Why-are-test-owner-reviews-required).
    20  
    21  2.  **Tast reviewers**. Send your change to tast-owners@google.com. In a few
    22      minutes, the code review is assigned to one or more [Tast reviewers] using
    23      the [gwsq] bot. Getting reviews from the Tast reviewers makes sure the
    24      change is good from the perspective of Tast test experts
    25      [(details)](#Why-are-Tast-reviewer-reviews-required).
    26  
    27  After getting LGTM from both reviewers, submit the change via the Commit Queue.
    28  
    29  [self-review checklist]: #Self_review-checklist
    30  [Tast reviewers]: https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/refs/heads/main/OWNERS
    31  [gwsq]: https://goto.google.com/gwsq-gerrit
    32  
    33  
    34  ## Self-review checklist
    35  
    36  ### Stabilize existing tests before adding new tests
    37  
    38  As [announced on the tast-users list], we have a policy that teams cannot add
    39  additional [mainline] tests until their existing informational tests have been
    40  stabilized and promoted to [critical] tests. Any new [mainline] test being added
    41  must have a clear plan for being promoted to a [critical] test.
    42  
    43  New test authors should start out by stabilizing an existing informational test.
    44  Doing so will expose them to Tast coding conventions, making it easier to write
    45  new code in the future.
    46  
    47  [announced on the tast-users list]: https://groups.google.com/a/chromium.org/d/topic/tast-users/dmS2OWp2bYU/discussion
    48  [mainline]: https://chromium.googlesource.com/chromiumos/platform/tast/+/HEAD/docs/test_attributes.md#manually_added-attributes
    49  [critical]: https://chromium.googlesource.com/chromiumos/platform/tast/+/HEAD/docs/test_attributes.md#manually_added-attributes
    50  
    51  ### Associate changes to bug tracker issues
    52  
    53  All Tast test changes should be associated with bug tracker issues (typically
    54  on the [Buganizer]). When adding a new test, it is recommended to
    55  file an issue to track promoting the test and using it to track flakiness
    56  issues that need to be resolved.
    57  
    58  [Buganizer]: https://b.corp.google.com/
    59  
    60  ### Do not make large changes
    61  
    62  (See also [go/small-cls])
    63  
    64  If you are working on a large test, don't send the entire test out as a single,
    65  gigantic change. Split your changes into smaller changes that can be submitted
    66  separately.
    67  
    68  Changes that consist of small, focused changes are always easier and faster to
    69  review than large, complicated changes. The length of a code review increases
    70  much faster than linearly with the size of the change.
    71  
    72  If your change is only large because of test data (e.g. baseline information
    73  about expected processes or files), it's fine to keep it all together.
    74  
    75  [go/small-cls]: https://goto.google.com/small-cls
    76  
    77  ### Run repo upload hooks
    78  
    79  Tast repositories are configured to run many linters ([gofmt], [goimports],
    80  [go vet], [golint] and [tast-lint]) as repo upload hooks to find obvious
    81  mistakes and style guide violations before time-consuming manual code reviews.
    82  
    83  Except for WIP changes, always make sure to run repo upload hooks. Changes
    84  failing to pass lint checks may not be reviewed.
    85  
    86  [gofmt]: https://golang.org/cmd/gofmt/
    87  [goimports]: https://godoc.org/golang.org/x/tools/cmd/goimports
    88  [go vet]: https://golang.org/cmd/vet/
    89  [golint]: https://github.com/golang/lint
    90  [tast-lint]: https://chromium.googlesource.com/chromiumos/platform/tast/+/HEAD/src/go.chromium.org/tast/core/cmd/tast-lint/
    91  
    92  ### Run unit tests
    93  
    94  Unit tests are not run automatically in repo upload hooks for technical reasons.
    95  You need to run them manually by the following command in the ChromeOS chroot:
    96  
    97  ```
    98  ~/trunk/src/platform/tast/fast_build.sh -T
    99  ```
   100  
   101  CLs breaking unit tests are rejected by the Commit Queue.
   102  
   103  ### Start a CQ dry run
   104  
   105  In the Gerrit UI, set the Commit-Queue+1 label to start a CQ dry run for your
   106  change. You do not have to wait for the dry run to finish before sending code
   107  reviews to reviewers.
   108  
   109  ### Check common code review comments
   110  
   111  Check the following documents for the most common comments made during code
   112  reviews.
   113  
   114  *   [Go Code Review Comments]
   115  *   [Tast: Code Review Comments]
   116  
   117  [Go Code Review Comments]: https://github.com/golang/go/wiki/CodeReviewComments
   118  [Tast: Code Review Comments]: code_review_comments.md
   119  
   120  ### Verify Need for Lacros Variant
   121  
   122  If a test utilizing the 'chrome' software dependency is modified or added the author needs to specify if a Lacros variant of the test is required. If a Lacros variant is needed, the author should add the Lacros variant of the test as part of the same review including the new/modified Ash-chrome test. The Ash-chrome test should then set the 'LacrosStatus' field to 'LacrosVariantExists'.
   123  
   124  If it is not possible to add a Lacros variant at review time, the 'LacrosStatus' field should be set to 'LacrosVariantNeeded'.
   125  
   126  If no Lacros variant is required, 'LacrosStatus' field should be set to 'LacrosVariantUnneeded'.
   127  
   128  New tests should *never* be set to 'LacrosVariantUnknown'. Either the Lacros variant of the test should be added, or value set to 'LacrosVariantUnneeded'.
   129  
   130  
   131  ## FAQ
   132  
   133  ### Why are test owner reviews required?
   134  
   135  Test owner reviews make sure your change is good from the perspective of feature
   136  experts.
   137  
   138  Test owners know the tested feature a lot better than Tast reviewers, obviously.
   139  
   140  ### Why are Tast reviewer reviews required?
   141  
   142  Tast reviewer reviews make sure your change is good from the perspective of Tast
   143  test experts.
   144  
   145  Tast reviewers are engineers from various teams in ChromeOS who have experience
   146  with writing and reviewing Tast tests. Their review insures that tests are
   147  written using learned best practices for test execution speed, stability and
   148  maintainability.
   149  
   150  ### Why are all Tast tests owned by Tast team, not by my team?
   151  
   152  Tast tests are in fact owned by the team listed in the Contacts field, not by
   153  the Tast team.
   154  
   155  It is simply due to technical reasons that the tast-tests repository's OWNERS
   156  file lists Tast reviewers only. Changes to a test should be reviewed by both
   157  the owning person/team listed in the test's Contacts field and Tast reviewers.
   158  
   159  ### Tests are failing in the Commit Queue. Can I skip Tast reviewer reviews for demoting/disabling them?
   160  
   161  Yes. In the case of emergency, please feel free to add
   162  `Exempt-From-Owner-Approval: <reason>` line to the change description to bypass
   163  Tast reviewer reviews.
   164  
   165  In any case, please remember to file a tracking bug for demotion/disablement and
   166  CC the change/bug to the test contacts listed in the Contacts field. If you need
   167  to chump a change, please get an approval from the sheriffs and leave a comment
   168  in Gerrit for reference.
   169  
   170  ### Can I send changes to a specific Tast reviewer?
   171  
   172  Yes in some cases.
   173  
   174  You can send changes to a specific Tast reviewer if you contact the reviewer in
   175  advance and they say okay.
   176  
   177  Also, when you are sending a stack of related changes to reviews, you may send
   178  only the first change to tast-owners@ and rest to the same reviewer.
   179  
   180  ### I am a Tast reviewer. Do I need approvals from other Tast reviewers for my own changes?
   181  
   182  No, you do not need approvals from other Tast reviewers. But please make sure
   183  to get LGTM from a test owner (or someone who knows the context if you are
   184  also a test owner).
   185  
   186  ### How can I become a Tast reviewer?
   187  
   188  Please write and review Tast changes to get used to Go, Tast and integration
   189  test best practices in general. Typically one writes more than 5 non-trivial
   190  changes before feeling familiar with Tast.
   191  
   192  Once you feel ready to go, please send a mail to tast-reviewers@google.com to
   193  join shadow reviews. See [go/tast-shadow-review] for details of the process.
   194  Upon graduating from shadow reviews, you will be added to [Tast reviewers].
   195  
   196  [go/tast-shadow-review]: https://goto.google.com/tast-shadow-review