This is an automated email from the ASF dual-hosted git repository. mergebot-role pushed a commit to branch mergebot in repository https://gitbox.apache.org/repos/asf/beam-site.git
commit a278295a04a66b8da720eed392572ae0bdacf66c Author: Mikhail Gryzykhin <[email protected]> AuthorDate: Tue Jul 24 14:10:04 2018 -0700 Add post-commit tests policies summary page. --- src/_includes/section-menu/contribute.html | 6 ++ src/contribute/postcommits-guides.md | 80 ++++++++++++++++++++++ src/contribute/postcommits-policies-details.md | 93 ++++++++++++++++++++++++++ src/contribute/postcommits-policies.md | 82 +++++++++++++++++++++++ src/contribute/testing.md | 81 ++++++++++++++++++++++ 5 files changed, 342 insertions(+) diff --git a/src/_includes/section-menu/contribute.html b/src/_includes/section-menu/contribute.html index 37e510c..07affbc 100644 --- a/src/_includes/section-menu/contribute.html +++ b/src/_includes/section-menu/contribute.html @@ -34,6 +34,12 @@ </ul> </li> <li> + <span class="section-nav-list-title">Policies</span> + <ul class="section-nav-list"> + <li><a href="{{ site.baseurl }}/contribute/postcommits-policies/">Post-commit tests policies</a></li> + </ul> +</li> +<li> <span class="section-nav-list-title">Committers</span> <ul class="section-nav-list"> diff --git a/src/contribute/postcommits-guides.md b/src/contribute/postcommits-guides.md new file mode 100644 index 0000000..efb91a6 --- /dev/null +++ b/src/contribute/postcommits-guides.md @@ -0,0 +1,80 @@ +--- +layout: section +title: 'Post-commit tests processes guides' +permalink: /contribute/postcommits-guides/ +section_menu: section-menu/contribute.html +--- +<!-- +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# Post-commit tests processes guides + +[TOC] + + +## Finding person to fix post-commit tests failures {#find_specialist} + +Finding proper person to triage the test failure might not be a trivial task. +However there are some guidelines. + +1. If you can do it -- go for it. +1. Check GitHub blame on files with problematic code. +1. Reach out to + [Slack chat](https://the-asf.slack.com/messages/C9H0YNP3P/apps/A0F7VRFKN/). +1. Reach out to [email protected]. + + +## Rolling back commit {#rollback} + +Most likely, you are on this page because there is a failing post-commit test +and you want to rollback culprit commit. + +Since rolling back someones change might be inconvenient for the person that +made the original change, we ask you to do some extra steps. These steps are +intended to help that person fix the issue that caused test failure and get his +feature back in line. + +1. Rollback the PR +1. Create JIRA task with information on why the code rolled back, links to + corresponding Tests failure task, triage information, any other relevant + information. Assign this task to original PR author. +1. Consider re-opening Jira ticket that was closed by original PR if any. +1. Send notification email with information of roll-back, links to original PR, + roll-back PR, reasons for roll-back to: + * [email protected] + * Original PR author and committer of that PR. +1. Close test-failure Jira task. Your work is done here. + + +## Disabling failing test {#disabling} + +Usually we want our tests green. If they eventually turn red, we fix them or do +rollback. + +However sometimes it might take us too much time to fix some specific test. In +this situation it might be feasible to disable a single test, so that the rest +of the suite give valid health signal. + +However this should be done with great caution, since disabling a test means +that we build upon shaky, not fully tested foundation. + +Because of this we want to do some extra precautions if we decide to follow this +path: + +* Notify everyone on dev list that some test is being disabled and describe + the problem. +* It is everyones job at this point to avoid pushing more code to the area + that failing test covered, since this code will not be properly tested at + this point. +* Do the fix and get the test back in line as soon as possible. diff --git a/src/contribute/postcommits-policies-details.md b/src/contribute/postcommits-policies-details.md new file mode 100644 index 0000000..6de0258 --- /dev/null +++ b/src/contribute/postcommits-policies-details.md @@ -0,0 +1,93 @@ +--- +layout: section +title: 'Post-commit policies details' +permalink: /contribute/postcommits-policies-details/ +section_menu: section-menu/contribute.html +--- +<!-- +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# Post-commit policies details + +A post-commit test failure means that there is a bug in the code. The longer the +bug exists, the harder it is to fix it due to ongoing code contributions. As a +result, we want to fix bugs quickly. The Beam community's post-commit test +policies help keep our code and test results in a good state. + + +## Rollback first {#rollback_first} + +Beam uses a "rollback first" approach: the first action to resolve a test +failure is to rollback the culprit code change. The two main benefits of this +approach are short implementation time and high reliability. When we rollback +first, we quickly return to a previously verified good state. + +At a high level, this approach consists of the following steps: + +1. Revert the culprit commit. +1. Re-run the post-commit tests to verify the tests pass. +1. Push the revert commit. + +For background on this policy, see the +[mailing list thread](https://lists.apache.org/thread.html/3bb4aa777751da2e2d7e22666aa6a2e18ae31891cb09d91718b75e74@%3Cdev.beam.apache.org%3E) +and [design doc](https://docs.google.com/document/d/1sczGwnCvdHiboVajGVdnZL0rfnr7ViXXAebBAf_uQME/edit). + + +## A failing test is a critical/P1 issue {#failing_test_is_critical_bug} + +It is difficult to properly verify new changes made on top of buggy code. In +some cases, adding additional code can make the problem worse. To avoid this +situation, fixing failing tests is our highest priority. + + +## A flaky test is a critical/P1 issue {#flake_is_failing} + +Flaky tests are considered failing tests, and fixing a flaky test is a +critical/P1 issue. + +Flaky tests are tests that randomly succeed or fail while using the same code +version. Flaky test failures are one of the most dangerous types of failures +because they are easy to ignore -- another run of the flaky test might pass +successfully. However, these failures can hide real bugs and flaky tests often +slowly accumulate. Someone must repeatedly triage the failures, and flaky tests +are often the hardest ones to fix. + +Flaky tests do not provide a reliable quality signal, so it is important to +quickly fix the flakiness. If a fix will take awhile to implement, it is safer +to disable the test until the fix is ready. + +Martin Fowler has a good [article](https://martinfowler.com/articles/nonDeterminism.html) +about non-determinism in tests. + + +## Flaky tests must be fixed or removed {#remove_flake} + +Flaky tests do not provide a reliable quality signal, which has a harmful effect +on all tests and can lead to a loss of trust in our test suite. As a result, +contributors might start to ignore test failures. + +We want everyone to trust our tests, so it is important to diligently fix all +flaky tests. If it is not possible to fix a flaky test, we must remove the test. + + +## Add new pre-commit tests as part of a post-commit fix {#precommit_for_postcommit} + +Post-commit tests are an important fail-safe, but we want to fail fast. Failing +fast means that we want to detect bugs in pre-commit tests, and _not_ in +post-commit tests. + +When you implement a fix for a post-commit test failure, add a new pre-commit +test that will detect similar failures in the future. For example, you can +implement a new unit test that covers a problematic code branch. + diff --git a/src/contribute/postcommits-policies.md b/src/contribute/postcommits-policies.md new file mode 100644 index 0000000..bd6609c --- /dev/null +++ b/src/contribute/postcommits-policies.md @@ -0,0 +1,82 @@ +--- +layout: section +title: 'Post-commit tests policies' +section_menu: section-menu/contribute.html +permalink: /contribute/postcommits-policies/ +--- +<!-- +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# Post-commit tests policies + +Post-commit tests validate that Beam works correctly in a live environment. The +tests also catch errors that are hard to predict in the design and +implementation stages + +Even though post-commit tests run after the code is merged into the repository, +it is important that the tests pass reliably. Jenkins executes post-commit tests +against the HEAD of the master branch. If post-commit tests fail, there is a +problem with the HEAD build. In addition, post-commit tests are time consuming +to run, and it is often hard to triage test failures. + + +## Policies {#policies} + +To ensure that Beam's post-commit tests are reliable and healthy, the Beam +community follows these post-commit test policies: + +* [Rollback first]({{ site.baseurl }}/contribute/postcommits-policies-details/index.html#rollback_first) +* [A failing test is a critical bug]({{ site.baseurl }}/contribute/postcommits-policies-details/index.html#failing_test_is_critical_bug) +* [A flaky test is a critical bug]({{ site.baseurl }}/contribute/postcommits-policies-details/index.html#flake_is_failing) +* [Flaky tests must either be fixed or removed]({{ site.baseurl }}/contribute/postcommits-policies-details/index.html#remove_flake) +* [Fixes for post-commit failures should include a corresponding new pre-commit test]({{ site.baseurl }}/contribute/postcommits-policies-details/index.html#precommit_for_postcommit) + + +## Post-commit test failure scenarios + +When a post-commit test fails, follow the provided steps for your situation. + +### I found a test failure {#found-failing-test} + +1. Create a [JIRA issue](https://issues.apache.org/jira/issues/?jql=project%20%3D%20BEAM%20AND%20component%20%3D%20test-failures) + and assign it to yourself. +1. Do high level triage of the failure. +1. [Assign the JIRA issue to a relevant person]({{ site.baseurl }}/contribute/postcommits-guides/index.html#find_specialist). + +### I was assigned a JIRA issue for a test failure {#assigned-failing-test} + +1. [Rollback the culprit change]({{ site.baseurl }}/contribute/postcommits-guides/index.md#rollback). +1. If you determine that rollback will take longer than 8 hours, [disable the + test temporarily]({{ site.baseurl }}/contribute/postcommits-guides/index.md#disabling) while you rollback or create a + fix. + +> Note: Rollback is always the first course of action. If a fix is trivial, +> open a pull request with the proposed fix while doing rollback. + +### My change was rolled back due to a test failure {#pr-rolled-back} + +1. Look at the JIRA issue to find the reason for the rollback. +1. Fix your code and re-run the post-commit tests. +1. Implement new pre-commit tests that will catch similar bugs before future + code is merged into the repository. +1. Open a new PR that contains your fix and the new pre-commit tests. + +## Useful links + +* [Best practices for writing tests]({{ site.baseurl }}/contribute/testing/index.md#best_practices) + +## References + +1. [Keeping post-commit tests green](https://lists.apache.org/thread.html/3bb4aa777751da2e2d7e22666aa6a2e18ae31891cb09d91718b75e74@%3Cdev.beam.apache.org%3E) + mailing list proposal thread. diff --git a/src/contribute/testing.md b/src/contribute/testing.md index 5002227..5fdd9ac 100644 --- a/src/contribute/testing.md +++ b/src/contribute/testing.md @@ -561,3 +561,84 @@ classes which reside under `"org[.]apache[.]beam[.].*Test.*"`, `"org[.]apache[.]beam[.].*IT"` or `"java[.]lang.*"`, belong to neither of the packages: `org.apache.beam.x`, `org.apache.beam.y`, `org.apache.beam.z`, nor equal to `Other.class`. + +## Best practices for writing tests {#best_practices} + +The following best practices help you to write reliable and maintainable tests. + +### Aim for one failure path + +An ideal test has one failure path. When you create your tests, minimize the +possible reasons for a test failure. A developer can debug a problem more +easily when there are fewer failure paths. + +### Avoid non-deterministic code + +Reliable tests are predictable and deterministic. Tests that contain +non-deterministic code are hard to debug and are often flaky. Non-deterministic +code includes the use of randomness, time, and multithreading. + +To avoid non-deterministic code, mock the corresponding methods or classes. + +### Use descriptive test names + +Helpful test names contain details about your test, such as test parameters and +the expected result. Ideally, a developer can read the test name and know where +the buggy code is and how to reproduce the bug. + +An easy and effective way to name your methods is to use these three questions: + +* What you are testing? +* What are the parameters of the test? +* What is the expected result of the test? + +For example, consider a scenario where you want to add a test for the +`Divide` method: + +```java +float Divide(float dividend, float divisor) { + return dividend / divisor; +} + +... + +@Test +void <--TestMethodName-->() { + assertThrows(Divide(10, 0)) +} +``` + +If you use a simple test name, such as `testDivide()`, you are missing important +information such as the expected action, parameter information, and expected +test result. As a result, triaging a test failure requires you to look at the +test implementation to see what the test does. + +Instead, use a name such as `invokingDivideWithDivisorEqualToZeroThrowsException()`, +which specifies: + +* the expected action of the test (`invokingDivide`) +* details about important parameters (the divisor is zero) +* the expected result (the test throws an exception) + +If this test fails, you can look at the descriptive test name to find the most +probable cause of the failure. In addition, test frameworks and test result +dashboards use the test name when reporting test results. Descriptive names +enable contributors to look at test suite results and easily see what +features are failing. + +Long method names are not a problem for test code. Test names are rarely used +(usually when you triage and debug), and when you do need to look at a +test, it is helpful to have descriptive names. + + +### Use a pre-commit test if possible + +Post-commit tests validate that Beam works correctly in broad variety of +scenarios. The tests catch errors that are hard to predict in the design and +implementation stages + +However, we often write a test to verify a specific scenario. In this situation, +it is usually possible to implement the test as a unit test or a component test. +You can add your unit tests or component tests to the pre-commit test suite, and +the pre-commit test results give you faster code health feedback during the +development stage, when a bug is cheap to fix.
