| Matthew Treinish | 16dd51b | 2014-06-11 12:04:51 -0400 | [diff] [blame] | 1 | Reviewing Tempest Code |
| 2 | ====================== |
| 3 | |
| 4 | To start read the `OpenStack Common Review Checklist |
| 5 | <https://wiki.openstack.org/wiki/ReviewChecklist#Common_Review_Checklist>`_ |
| 6 | |
| 7 | |
| 8 | Ensuring code is executed |
| 9 | ------------------------- |
| 10 | |
| 11 | For any new or change to a test it has to be verified in the gate. This means |
| 12 | that the first thing to check with any change is that a gate job actually runs |
| 13 | it. Tests which aren't executed either because of configuration or skips should |
| 14 | not be accepted. |
| 15 | |
| 16 | |
| 17 | Unit Tests |
| 18 | ---------- |
| 19 | |
| 20 | For any change that adds new functionality to either common functionality or an |
| 21 | out-of-band tool unit tests are required. This is to ensure we don't introduce |
| 22 | future regressions and to test conditions which we may not hit in the gate runs. |
| 23 | Tests, and service clients aren't required to have unit tests since they should |
| 24 | be self verifying by running them in the gate. |
| 25 | |
| 26 | |
| 27 | API Stability |
| 28 | ------------- |
| 29 | Tests should only be added for a published stable APIs. If a patch contains |
| 30 | tests for an API which hasn't been marked as stable or for an API that which |
| 31 | doesn't conform to the `API stability guidelines |
| 32 | <https://wiki.openstack.org/wiki/Governance/Approved/APIStability>`_ then it |
| 33 | should not be approved. |
| 34 | |
| 35 | |
| 36 | Reject Copy and Paste Test Code |
| 37 | ------------------------ |
| 38 | When creating new tests that are similar to existing tests it is tempting to |
| 39 | simply copy the code and make a few modifications. This increases code size and |
| 40 | the maintenance burden. Such changes should not be approved if it is easy to |
| 41 | abstract the duplicated code into a function or method. |
| 42 | |
| 43 | |
| 44 | Being explicit |
| 45 | -------------- |
| 46 | When tests are being added that depend on a configurable feature or extension, |
| 47 | polling the API to discover that it is enabled should not be done. This will |
| 48 | just result in bugs being masked because the test can be skipped automatically. |
| 49 | Instead the config file should be used to determine whether a test should be |
| 50 | skipped or not. Do not approve changes that depend on an API call to determine |
| 51 | whether to skip or not. |
| 52 | |
| 53 | |
| 54 | When to approve |
| 55 | --------------- |
| 56 | * Every patch needs two +2s before being approved. |
| 57 | * Its ok to hold off on an approval until a subject matter expert reviews it |
| 58 | * If a patch has already been approved but requires a trivial rebase to merge, |
| 59 | you do not have to wait for a second +2, since the patch has already had |
| 60 | two +2s. |