zhongyujiang commented on code in PR #7131:
URL: https://github.com/apache/iceberg/pull/7131#discussion_r1142032725
##########
CONTRIBUTING.md:
##########
@@ -194,3 +194,83 @@ When passing boolean arguments to existing or external
methods, use inline comme
* For example, preferred convection `access-key-id` rather than
`access.key.id`
2. Use `.` to create a hierarchy of config groups
* For example, `s3` in `s3.access-key-id`, `s3.secret-access-key`
+
+## Testing
+
+### AssertJ
+
+Prefer using [AssertJ](https://github.com/assertj/assertj) assertions as those
provide a rich and intuitive set of strongly-typed assertions.
+Checks can be expressed in a fluent way and
[AssertJ](https://github.com/assertj/assertj) provides rich context when
assertions fail.
+Additionally, [AssertJ](https://github.com/assertj/assertj) has powerful
testing capabilities on collections and exceptions.
+Please refer to the [usage
guide](https://assertj.github.io/doc/#assertj-core-assertions-guide) for
additional examples.
+
+```java
+// bad: will only say true != false when check fails
+assertTrue(x instanceof Xyz);
+
+// better: will show type of x when check fails
+assertThat(x).isInstanceOf(Xyz.class);
+
+// bad: will only say true != false when check fails
+assertTrue(catalog.listNamespaces().containsAll(expected));
+
+// better: will show content of expected and of catalog.listNamespaces() if
check fails
+assertThat(catalog.listNamespaces()).containsAll(expected);
+```
+```java
+// ok
+assertNotNull(metadataFileLocations);
+assertEquals(metadataFileLocations.size(), 4);
+
+// better: will show the content of metadataFileLocations if check fails
+assertThat(metadataFileLocations).isNotNull().hasSize(4);
+
+// or
+assertThat(metadataFileLocations).isNotNull().hasSameSizeAs(expected).hasSize(4);
+```
+
+```java
+// bad
+try {
+ catalog.createNamespace(deniedNamespace);
+ Assert.fail("this should fail");
+} catch (Exception e) {
+ assertEquals(AccessDeniedException.class, e.getClass());
+ assertEquals("User 'testUser' has no permission to create namespace",
e.getMessage());
+}
+
+// better
+assertThatThrownBy(() -> catalog.createNamespace(deniedNamespace))
+ .isInstanceOf(AccessDeniedException.class)
+ .hasMessage("User 'testUser' has no permission to create namespace");
+```
+Checks on exceptions should always make sure to assert that a particular
exception message has occurred.
+
+
+### Awaitility
+
+Avoid using `Thread.sleep()` in tests as it leads to long test durations and
flaky behavior if a condition takes slightly longer than expected.
+
+```java
+deleteTablesAsync();
+Thread.sleep(3000L);
+assertThat(tables()).isEmpty()
Review Comment:
Nit: missing a semicolon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
