This is an automated email from the ASF dual-hosted git repository. chesnay pushed a commit to branch asf-site in repository https://gitbox.apache.org/repos/asf/flink-web.git
commit 62b6d2fc82f0aa5cd6a2a2122aa475f944733178 Author: Chesnay Schepler <[email protected]> AuthorDate: Thu Nov 25 12:14:39 2021 +0100 Rebuild website --- .../code-style-and-quality-common.html | 126 +++++++++++++-------- 1 file changed, 77 insertions(+), 49 deletions(-) diff --git a/content/contributing/code-style-and-quality-common.html b/content/contributing/code-style-and-quality-common.html index 1d8adba..b69be6d 100644 --- a/content/contributing/code-style-and-quality-common.html +++ b/content/contributing/code-style-and-quality-common.html @@ -280,6 +280,13 @@ </li> <li><a href="#concurrency-and-threading" id="markdown-toc-concurrency-and-threading">5. Concurrency and Threading</a></li> <li><a href="#dependencies-and-modules" id="markdown-toc-dependencies-and-modules">6. Dependencies and Modules</a></li> + <li><a href="#testing" id="markdown-toc-testing">7. Testing</a> <ul> + <li><a href="#tooling" id="markdown-toc-tooling">Tooling</a></li> + <li><a href="#write-targeted-tests" id="markdown-toc-write-targeted-tests">Write targeted tests</a></li> + <li><a href="#avoid-mockito---use-reusable-test-implementations" id="markdown-toc-avoid-mockito---use-reusable-test-implementations">Avoid Mockito - Use reusable test implementations</a></li> + <li><a href="#avoid-timeouts-in-junit-tests" id="markdown-toc-avoid-timeouts-in-junit-tests">Avoid timeouts in JUnit tests</a></li> + </ul> + </li> </ul> </div> @@ -493,55 +500,6 @@ then the S3 access should be factored out into an interface and test should repl <p>⇒ Please note that these steps often require more effort in implementing tests (factoring out interfaces, creating dedicated test stubs), but make the tests more resilient to changes in other components, i.e., you do not need to touch the tests when making unrelated changes.</p> -<p><strong>Write targeted tests</strong></p> - -<ul> - <li><span style="text-decoration:underline;">Test contracts not implementations</span>: Test that after a sequence of actions, the components are in a certain state, rather than testing that the components followed a sequence of internal state modifications. - <ul> - <li>For example, a typical antipattern is to check whether one specific method was called as part of the test</li> - </ul> - </li> - <li> - <p>A way to enforce this is to try to follow the <em>Arrange</em>, <em>Act</em>, <em>Assert</em> test structure when writing a unit test (<a href="https://xp123.com/articles/3a-arrange-act-assert/";>https://xp123.com/articles/3a-arrange-act-assert/</a>)</p> - - <p>This helps to communicate the intention of the test (what is the scenario under test) rather than the mechanics of the tests. The technical bits go to a static methods at the bottom of the test class.</p> - - <p>Example of tests in Flink that follow this pattern are:</p> - - <ul> - <li><a href="https://github.com/apache/flink/blob/master/flink-core/src/test/java/org/apache/flink/util/LinkedOptionalMapTest.java";>https://github.com/apache/flink/blob/master/flink-core/src/test/java/org/apache/flink/util/LinkedOptionalMapTest.java</a></li> - <li><a href="https://github.com/apache/flink/blob/master/flink-filesystems/flink-s3-fs-base/src/test/java/org/apache/flink/fs/s3/common/writer/RecoverableMultiPartUploadImplTest.java";>https://github.com/apache/flink/blob/master/flink-filesystems/flink-s3-fs-base/src/test/java/org/apache/flink/fs/s3/common/writer/RecoverableMultiPartUploadImplTest.java</a></li> - </ul> - </li> -</ul> - -<p><strong>Avoid Mockito - Use reusable test implementations</strong></p> - -<ul> - <li>Mockito-based tests tend to be costly to maintain in the long run by encouraging duplication of functionality and testing for implementation rather than effect - <ul> - <li>More details: <a href="https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8";>https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8</a></li> - </ul> - </li> - <li>Instead, create reusable test implementations and utilities - <ul> - <li>That way, when some class changes, we only have to update a few test utils or mocks</li> - </ul> - </li> -</ul> - -<p><strong>Avoid timeouts in JUnit tests</strong></p> - -<p>Generally speaking, we should avoid setting local timeouts in JUnit tests but rather depend on the -global timeout in Azure. The global timeout benefits from taking thread dumps just before timing out -the build, easing debugging.</p> - -<p>At the same time, any timeout value that you manually set is arbitrary. If it’s set too low, you get -test instabilities. What too low means depends on numerous factors, such as hardware and current -utilization (especially I/O). Moreover, a local timeout is more maintenance-intensive. It’s one more -knob where you can tweak a build. If you change the test a bit, you also need to double-check the -timeout. Hence, there have been quite a few commits that just increase timeouts.</p> - <h3 id="performance-awareness">Performance Awareness</h3> <p>We can conceptually distinguish between code that “coordinates” and code that “processes data”. Code that coordinates should always favor simplicity and cleanness. Data processing code is highly performance critical and should optimize for performance.</p> @@ -649,6 +607,76 @@ Examples are in the RPC system, Network Stack, in the Task’s mailbox model, or </li> </ul> +<h2 id="testing">7. Testing</h2> + +<h3 id="tooling">Tooling</h3> + +<p>We are moving our codebase to <a href="https://junit.org/junit5/docs/current/user-guide/";>JUnit 5</a> and <a href="https://assertj.github.io/doc/";>AssertJ</a> as our testing framework and assertions library of choice.</p> + +<p>Unless there is a specific reason, make sure you use JUnit 5 and AssertJ when contributing to Flink with new tests and even when modifying existing tests. Don’t use Hamcrest, JUnit assertions and <code>assert</code> directive. +Make your tests readable and don’t duplicate assertions logic provided by AssertJ or by <a href="https://assertj.github.io/doc/#assertj-core-custom-assertions";>custom assertions</a> provided by some flink modules. +For example, avoid:</p> + +<div class="highlight"><pre><code class="language-java"><span class="k">assert</span> <span class="n">list</span><span class="o">.</span><span class="na">size</span><span class="o">()</span> <span class="o">==</span> <span class="mi">10</span><span class="o">;</span> +<span class="k">for</span> <span class="o">(</span><span class="n">String</span> <span class="n">item</span> <span class="o">:</span> <span class="n">list</span><span class="o">)</span> <span class="o">{</span> + <span class="n">assertTrue</span><span class="o">(</span><span class="n">item</span><span class="o">.</span><span class="na">length</span><span class="o">()</span> <span class="o"><</span> <span class="mi">10</span><span class="o">);</span> +<span class="o">}</span></code></pre></div> + +<p>And instead use:</p> + +<div class="highlight"><pre><code class="language-java"><span class="n">assertThat</span><span class="o">(</span><span class="n">list</span><span class="o">)</span> + <span class="o">.</span><span class="na">hasSize</span><span class="o">(</span><span class="mi">10</span><span class="o">)</span> + <span class="o">.</span><span class="na">allMatch</span><span class="o">(</span><span class="n">item</span> <span class="o">-></span> <span class="n">item</span><span class="o">.</span><span class="na">length</span><span class="o">()</span> <span class="o"><</span> <span class="mi">10</span><span class="o">);</span></code></pre></div> + +<h3 id="write-targeted-tests">Write targeted tests</h3> + +<ul> + <li><span style="text-decoration:underline;">Test contracts not implementations</span>: Test that after a sequence of actions, the components are in a certain state, rather than testing that the components followed a sequence of internal state modifications. + <ul> + <li>For example, a typical antipattern is to check whether one specific method was called as part of the test</li> + </ul> + </li> + <li> + <p>A way to enforce this is to try to follow the <em>Arrange</em>, <em>Act</em>, <em>Assert</em> test structure when writing a unit test (<a href="https://xp123.com/articles/3a-arrange-act-assert/";>https://xp123.com/articles/3a-arrange-act-assert/</a>)</p> + + <p>This helps to communicate the intention of the test (what is the scenario under test) rather than the mechanics of the tests. The technical bits go to a static methods at the bottom of the test class.</p> + + <p>Example of tests in Flink that follow this pattern are:</p> + + <ul> + <li><a href="https://github.com/apache/flink/blob/master/flink-core/src/test/java/org/apache/flink/util/LinkedOptionalMapTest.java";>https://github.com/apache/flink/blob/master/flink-core/src/test/java/org/apache/flink/util/LinkedOptionalMapTest.java</a></li> + <li><a href="https://github.com/apache/flink/blob/master/flink-filesystems/flink-s3-fs-base/src/test/java/org/apache/flink/fs/s3/common/writer/RecoverableMultiPartUploadImplTest.java";>https://github.com/apache/flink/blob/master/flink-filesystems/flink-s3-fs-base/src/test/java/org/apache/flink/fs/s3/common/writer/RecoverableMultiPartUploadImplTest.java</a></li> + </ul> + </li> +</ul> + +<h3 id="avoid-mockito---use-reusable-test-implementations">Avoid Mockito - Use reusable test implementations</h3> + +<ul> + <li>Mockito-based tests tend to be costly to maintain in the long run by encouraging duplication of functionality and testing for implementation rather than effect + <ul> + <li>More details: <a href="https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8";>https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8</a></li> + </ul> + </li> + <li>Instead, create reusable test implementations and utilities + <ul> + <li>That way, when some class changes, we only have to update a few test utils or mocks</li> + </ul> + </li> +</ul> + +<h3 id="avoid-timeouts-in-junit-tests">Avoid timeouts in JUnit tests</h3> + +<p>Generally speaking, we should avoid setting local timeouts in JUnit tests but rather depend on the +global timeout in Azure. The global timeout benefits from taking thread dumps just before timing out +the build, easing debugging.</p> + +<p>At the same time, any timeout value that you manually set is arbitrary. If it’s set too low, you get +test instabilities. What too low means depends on numerous factors, such as hardware and current +utilization (especially I/O). Moreover, a local timeout is more maintenance-intensive. It’s one more +knob where you can tweak a build. If you change the test a bit, you also need to double-check the +timeout. Hence, there have been quite a few commits that just increase timeouts.</p> + <hr /> <div class="footnotes">
