Felipe Monteiro | eb197db | 2018-06-10 12:21:49 -0400 | [diff] [blame] | 1 | Reviewing Patrole Code |
| 2 | ====================== |
| 3 | To start read the `OpenStack Common Review Checklist |
| 4 | <https://docs.openstack.org/infra/manual/developers.html#peer-review>`_ |
| 5 | |
| 6 | |
| 7 | Ensuring code is executed |
| 8 | ------------------------- |
| 9 | Any new test or change to an existing test has to be verified in the gate. This |
| 10 | means that the first thing to check with any change is that a gate job actually |
| 11 | runs it. Tests which aren't executed either because of configuration or skips |
| 12 | should not be accepted. |
| 13 | |
| 14 | |
| 15 | Execution time |
| 16 | -------------- |
| 17 | Along with checking that the jobs log that a new test is actually executed, |
| 18 | also pay attention to the execution time of that test. Patrole already runs |
| 19 | hundreds of tests per job in its check and gate pipelines and it is important |
| 20 | that the overall runtime of the jobs be constrained as much as possible. |
| 21 | Consider applying the ``@decorators.attr(type='slow')`` |
| 22 | `test attribute decorator`_ to a test if its runtime is longer than 30 seconds. |
| 23 | |
| 24 | .. _test attribute decorator: https://docs.openstack.org/tempest/latest/HACKING.html#test-attributes |
| 25 | |
| 26 | |
| 27 | Unit Tests |
| 28 | ---------- |
| 29 | For any change that adds new functionality to common functionality unit tests |
| 30 | are required. This is to ensure we don't introduce future regressions and to |
| 31 | test conditions which we may not hit in the gate runs. |
| 32 | |
| 33 | |
| 34 | API Stability |
| 35 | ------------- |
| 36 | Tests should only be added for published stable APIs. If a patch contains |
| 37 | tests for an API which hasn't been marked as stable or for an API which |
| 38 | doesn't conform to the `API stability guidelines |
| 39 | <https://wiki.openstack.org/wiki/Governance/Approved/APIStability>`_ then it |
| 40 | should not be approved. |
| 41 | |
| 42 | Similarly, tests should only be added for policies that are covered by |
| 43 | `policy in code documentation |
| 44 | <https://specs.openstack.org/openstack/keystone-specs/specs/keystone/pike/policy-in-code.html>`_. |
| 45 | Any existing tests that test policies not covered by such documentation |
| 46 | are either: |
| 47 | |
| 48 | * part of a service that has not yet migrated to policy in code; or |
| 49 | * legacy in the sense that they were created prior to policy in code |
| 50 | |
| 51 | For the first bullet, the tests should not be considered stable, but should be |
| 52 | kept around to maintain coverage. These tests are a best-effort attempt at |
| 53 | offering RBAC test coverage for the service that has not yet migrated to |
| 54 | policy in code. |
| 55 | |
| 56 | For the second bullet, the tests should be updated to conform to policy in |
| 57 | code documentation, if applicable. |
| 58 | |
| 59 | |
| 60 | Reject Copy and Paste Test Code |
| 61 | ------------------------------- |
| 62 | When creating new tests that are similar to existing tests it is tempting to |
| 63 | simply copy the code and make a few modifications. This increases code size and |
| 64 | the maintenance burden. Such changes should not be approved if it is easy to |
| 65 | abstract the duplicated code into a function or method. |
| 66 | |
| 67 | |
| 68 | Tests overlap |
| 69 | ------------- |
| 70 | When a new test is being proposed, question whether this feature is not already |
| 71 | tested with Patrole. Patrole has more than 600 tests, spread amongst many |
| 72 | directories, so it's easy to introduce test duplication. |
| 73 | |
| 74 | Test Duplication |
| 75 | ^^^^^^^^^^^^^^^^ |
| 76 | |
| 77 | Test duplication means: |
| 78 | |
| 79 | * testing an API endpoint in more than one test |
| 80 | * testing the same policy in more than one test |
| 81 | |
| 82 | For the first bullet, try to avoid calling the same API inside the |
| 83 | ``self.rbac_utils.override_role`` call. |
| 84 | |
| 85 | .. note:: |
| 86 | |
| 87 | If the same API is tested against different policies, consider combining |
| 88 | the different tests into only 1 test, that tests the API against all |
| 89 | the policies it enforces. |
| 90 | |
| 91 | For the second bullet, try to avoid testing the same policies across multiple |
| 92 | tests. |
| 93 | |
| 94 | .. note:: |
| 95 | |
| 96 | This is not always possible since policy granularity doesn't exist for all |
| 97 | APIs. In cases where policy granularity doesn't exist, make sure that the |
| 98 | policy overlap only exists for the non-granular APIs that enforce the same |
| 99 | policy. |
| 100 | |
| 101 | |
| 102 | Being explicit |
| 103 | -------------- |
| 104 | When tests are being added that depend on a configurable feature or extension, |
| 105 | polling the API to discover that it is enabled should not be done. This will |
| 106 | just result in bugs being masked because the test can be skipped automatically. |
| 107 | Instead the config file should be used to determine whether a test should be |
| 108 | skipped or not. Do not approve changes that depend on an API call to determine |
| 109 | whether to skip or not. |
| 110 | |
| 111 | |
| 112 | Release Notes |
| 113 | ------------- |
| 114 | Release notes are how we indicate to users and other consumers of Patrole what |
| 115 | has changed in a given release. There are certain types of changes that |
| 116 | require release notes and we should not approve them without including a release |
| 117 | note. These include but aren't limited to, any addition, deprecation or removal |
| 118 | from the framework code, any change to configuration options (including |
| 119 | deprecation), major feature additions, and anything backwards incompatible or |
| 120 | would require a user to take note or do something extra. |
| 121 | |
| 122 | |
| 123 | Deprecated Code |
| 124 | --------------- |
| 125 | Sometimes we have some bugs in deprecated code. Basically, we leave it. Because |
| 126 | we don't need to maintain it. However, if the bug is critical, we might need to |
| 127 | fix it. When it will happen, we will deal with it on a case-by-case basis. |
| 128 | |
| 129 | |
| 130 | When to approve |
| 131 | --------------- |
| 132 | * Every patch needs two +2's before being approved. |
| 133 | * It's OK to hold off on an approval until a subject matter expert reviews it. |
| 134 | * If a patch has already been approved but requires a trivial rebase to merge, |
| 135 | you do not have to wait for a second +2, since the patch has already had |
| 136 | two +2's. |