始めに、 OpenStack Common Review Checklist を読むこと
新たなテスト作成、あるいは変更に対しては、 gate での確認が必要である。これは、 変更に対するチェックとして始めにすべきことは、 gate job が実際に動作している ことを確認するということを意味している。コンフィグレーションやスキップなどの理 由でテストが実行されていない場合、受け入れられるべきではない。
もし、新たなテストが新たなコンフィグオプション（機能のフラグのような）に依存し て追加された場合、そのコミットメッセージは、新たに導入されたテストの実行を可能 とするために、関連する DevStack もしくは、DevStack-Gate の変更を参照しなければ ならない。この参照は、 Cross-Repository Dependency もしくは、 Gerrit review へのシンプルなリンクで可能である。
新たなテストが実際に実行されている job ログの確認をする際、そのテストの実行時 間についても注意を払うこと。個々のテストが、毎日、数百回実行されることを忘れて はならない。なぜならば、 Tempest のテストは多くの OpenStackプロジェクトにおい て実行されるためである。その新たなテストが、どの程度のコストで、テストにおける その機能が、どれだけ重要でクリティカルなのかを考慮することは価値がある。
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.
API and scenario tests aren't required to have unit tests since they should
be self-verifying by running them in the gate. All service clients, on the
other hand, must have unit tests, as they belong to tempest/lib.
テストは、公開された stable な API に対してのみ追加されるべきである。もし、 パッチが、 stable とマークされてない、もしくは、 API が API stability guidelines に合致していない場合、そのパッチは approve されるべきではない。
既存テストと似たような新たなテストを作る際、既存コードをコピーして、少し修正す るだけにしたくなりがちである。これは、コードサイズとメンテナンス労力を増大させ てしまう。したがって、このような変更は approve されるべきではなく、もし、 簡単に重複コードを関数もしくはメソッドにして抽象化できるなら、そうすべきであ る。
新たなテストが提案された際、すでに、 Tempest にその機能がテストされているかど うか問うべきである。Tempest には、1,200を超えるテストがあり、多くのディレクト リに分散している。そのため、容易にテストの重複が発生してしまう。例えば、 ボリュームをサーバにアタッチするテストは、見方によって、 compute のテストでも あるし、 volume のテストでもありえる。そのため、全コードベースを注意深く、可能 な限り重複がないか探す必要がある。大まかに言えば、古めの機能は大抵、既にテスト されているだろう。
テストがコンフィグ可能な機能や拡張に依存して追加された時、 API 呼び出しでその 機能が有効であると判断すべきではない。これは、単にバグを覆い隠してしまう結果と なる。なぜならば、そのテストは自動的にスキップされてしまうからである。 代わりに、コンフィグファイルを使い、テストがスキップされるべきか否かを決定すべ きである。API呼び出しに依存して、スキップするか否かを行う変更は、 approve してはならない。
With the introduction of the Tempest external test plugin interface we needed to provide a stable contract for Tempest's configuration options. This means we can no longer simply remove a configuration option when it's no longer used. Patches proposed that remove options without a deprecation cycle should not be approved. Similarly when changing default values with configuration we need to similarly be careful that we don't break existing functionality. Also, when adding options, just as before, we need to weigh the benefit of adding an additional option against the complexity and maintenance overhead having it costs.
When a new test is being added refer to the :ref:`TestDocumentation` section in hacking to see if the requirements are being met. With the exception of a class level docstring linking to the API ref doc in the API tests and a docstring for scenario tests this is up to the reviewers discretion whether a docstring is required or not.
Make sure that any test that is renamed, relocated (e.g. moved to another class), or removed does not belong to the interop testing suite -- which includes a select suite of Tempest tests for the purposes of validating that OpenStack vendor clouds are interoperable -- or a project's whitelist or blacklist files.
It is of critical importance that no interop, whitelist or blacklist test reference be broken by a patch set introduced to Tempest that renames, relocates or removes a referenced test.
Please check the existence of code which references Tempest tests with: http://codesearch.openstack.org/
Make sure that modifications to an interop test are backwards-compatible. This means that code modifications to tests should not undermine the quality of the validation currently performed by the test or significantly alter the behavior of the test.
Reference the :ref:`test-removal` guidelines for understanding best practices associated with test removal.
Release notes are how we indicate to users and other consumers of Tempest what has changed in a given release. Since Tempest 10.0.0 we've been using reno to manage and build the release notes. There are certain types of changes that require release notes and we should not approve them without including a release note. These include but aren't limited to, any addition, deprecation or removal from the lib interface, any change to configuration options (including deprecation), CLI additions or deprecations, major feature additions, and anything backwards incompatible or would require a user to take note or do something extra.
Sometimes we have some bugs in deprecated code. Basically, we leave it. Because we don't need to maintain it. However, if the bug is critical, we might need to fix it. When it will happen, we will deal with it on a case-by-case basis.
It's OK to hold off on an approval until a subject matter expert reviews it.
Every patch needs two +2's before being approved.
However, a single Tempest core reviewer can approve patches without waiting for another +2 in the following cases:
If a patch has already been approved but requires a trivial rebase to merge, then there is no need to wait for a second +2, since the patch has already had two +2's.
If any trivial patch set fixes one of the items below:
- Documentation or code comment typo
- Documentation ref link
- Example: example
Note
Any other small documentation, CI job, or code change does not fall under this category.
If the patch unblocks a failing project gate, provided that:
- the project's PTL +1's the change
- the patch does not affect any other project's testing gates
- the patch does not cause any negative side effects
Note that such a policy should be used judiciously, as we should strive to have two +2's on each patch set, prior to approval.