[GitHub] [iceberg] nastra commented on a diff in pull request #7131: Contributing Docs: Add section for testing code

["nastra (via GitHub)" <gi...@apache.org>]

nastra commented on code in PR #7131:
URL: https://github.com/apache/iceberg/pull/7131#discussion_r1141738191


##########
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("access denied", e.getMessage());
+}
+
+// better
+assertThatThrownBy(() -> catalog.createNamespace(deniedNamespace))
+    .isInstanceOf(AccessDeniedException.class)
+    .hasMessage("access denied");

Review Comment:
   makes sense, done



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org