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 92528d2b54996ed585e36a1bb26e595a35b71e0a Author: slinkydeveloper <[email protected]> AuthorDate: Mon Nov 22 15:23:45 2021 +0100 [hotfix] Move some sections from "Design for Testability" to "Testing" Signed-off-by: slinkydeveloper <[email protected]> --- contributing/code-style-and-quality-common.md | 67 +++++++++++++-------------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/contributing/code-style-and-quality-common.md b/contributing/code-style-and-quality-common.md index 3851ad8..a2a8998 100644 --- a/contributing/code-style-and-quality-common.md +++ b/contributing/code-style-and-quality-common.md @@ -211,40 +211,6 @@ then the S3 access should be factored out into an interface and test should repl ⇒ 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. - -**Write targeted tests** - -* <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. - * For example, a typical antipattern is to check whether one specific method was called as part of the test -* A way to enforce this is to try to follow the _Arrange_, _Act_, _Assert_ test structure when writing a unit test ([https://xp123.com/articles/3a-arrange-act-assert/](https://xp123.com/articles/3a-arrange-act-assert/)) - - 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. - - Example of tests in Flink that follow this pattern are: - - * [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) - * [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) - - -**Avoid Mockito - Use reusable test implementations** - -* 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 - * More details: [https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8](https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8) -* Instead, create reusable test implementations and utilities - * That way, when some class changes, we only have to update a few test utils or mocks - -**Avoid timeouts in JUnit tests** - -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. - -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. - ### Performance Awareness 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. @@ -333,6 +299,8 @@ Examples are in the RPC system, Network Stack, in the Task’s mailbox model, or ## 7. Testing +### Tooling + We are moving our codebase to [JUnit 5](https://junit.org/junit5/docs/current/user-guide/) and [AssertJ](https://assertj.github.io/doc/) as our testing framework and assertions library of choice. 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 `assert` directive. @@ -354,7 +322,38 @@ assertThat(list) .allMatch(item -> item.length() < 10); ``` +### Write targeted tests + +* <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. + * For example, a typical antipattern is to check whether one specific method was called as part of the test +* A way to enforce this is to try to follow the _Arrange_, _Act_, _Assert_ test structure when writing a unit test ([https://xp123.com/articles/3a-arrange-act-assert/](https://xp123.com/articles/3a-arrange-act-assert/)) + + 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. + + Example of tests in Flink that follow this pattern are: + + * [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) + * [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) + + +### Avoid Mockito - Use reusable test implementations + +* 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 + * More details: [https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8](https://docs.google.com/presentation/d/1fZlTjOJscwmzYadPGl23aui6zopl94Mn5smG-rB0qT8) +* Instead, create reusable test implementations and utilities + * That way, when some class changes, we only have to update a few test utils or mocks + +### Avoid timeouts in JUnit tests + +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. +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.
