diff --git a/REVIEWING.rst b/REVIEWING.rst new file mode 100644 index 0000000000..d6dc83ee59 --- /dev/null +++ b/REVIEWING.rst @@ -0,0 +1,60 @@ +Reviewing Tempest Code +====================== + +To start read the `OpenStack Common Review Checklist +`_ + + +Ensuring code is executed +------------------------- + +For any new or change to a test it has to be verified in the gate. This means +that the first thing to check with any change is that a gate job actually runs +it. Tests which aren't executed either because of configuration or skips should +not be accepted. + + +Unit Tests +---------- + +For any change that adds new functionality to either common functionality or an +out-of-band tool unit tests are required. This is to ensure we don't introduce +future regressions and to test conditions which we may not hit in the gate runs. +Tests, and service clients aren't required to have unit tests since they should +be self verifying by running them in the gate. + + +API Stability +------------- +Tests should only be added for a published stable APIs. If a patch contains +tests for an API which hasn't been marked as stable or for an API that which +doesn't conform to the `API stability guidelines +`_ then it +should not be approved. + + +Reject Copy and Paste Test Code +------------------------ +When creating new tests that are similar to existing tests it is tempting to +simply copy the code and make a few modifications. This increases code size and +the maintenance burden. Such changes should not be approved if it is easy to +abstract the duplicated code into a function or method. + + +Being explicit +-------------- +When tests are being added that depend on a configurable feature or extension, +polling the API to discover that it is enabled should not be done. This will +just result in bugs being masked because the test can be skipped automatically. +Instead the config file should be used to determine whether a test should be +skipped or not. Do not approve changes that depend on an API call to determine +whether to skip or not. + + +When to approve +--------------- + * Every patch needs two +2s before being approved. + * Its ok to hold off on an approval until a subject matter expert reviews it + * If a patch has already been approved but requires a trivial rebase to merge, + you do not have to wait for a second +2, since the patch has already had + two +2s. diff --git a/doc/source/REVIEWING.rst b/doc/source/REVIEWING.rst new file mode 120000 index 0000000000..841e042c55 --- /dev/null +++ b/doc/source/REVIEWING.rst @@ -0,0 +1 @@ +../../REVIEWING.rst \ No newline at end of file diff --git a/doc/source/index.rst b/doc/source/index.rst index 25bc9005cd..d3118ac3fb 100644 --- a/doc/source/index.rst +++ b/doc/source/index.rst @@ -9,6 +9,7 @@ Contents: overview HACKING + REVIEWING ------------ Field Guides