adoroszlai commented on code in PR #5862:
URL: https://github.com/apache/ozone/pull/5862#discussion_r1436157576
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestJsonUtils.java:
##########
@@ -46,7 +46,8 @@ public void printObjectAsJson() throws IOException {
}
private static void assertContains(String str, String part) {
- assertTrue(str.contains(part),
- "Expected JSON to contain '" + part + "', but didn't: " + str);
+ assertThat(str)
+ .withFailMessage("Expected JSON to contain '" + part + "', but didn't:
" + str)
Review Comment:
I think `withFailMessage` can be omitted here, too.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -296,8 +297,7 @@ public void testRevokeCertificates() throws Exception {
CRLReason.lookup(CRLReason.keyCompromise), now);
result.get();
});
- assertTrue(execution.getCause().getMessage()
- .contains("Certificates cannot be null"));
+ assertThat(execution.getCause().getMessage()).contains("Certificates
cannot be null");
Review Comment:
nit: keeping `.contains...` on new line makes the assertion more readable,
because `assertThat`'s argument is also a chained method call
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/TestSecretKeyManager.java:
##########
@@ -110,16 +110,15 @@ public void testLoadSecretKeys(List<ManagedSecretKey>
savedSecretKey,
// expect the current key is newly generated.
assertFalse(savedSecretKey.contains(state.getCurrentKey()));
Review Comment:
This one can also be converted to `assertThat`.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java:
##########
@@ -183,11 +183,11 @@ public void verifyDefaultLogLevelForAuthFailure() throws
IOException {
@Test
public void messageIncludesAllParts() {
String message = WRITE_FAIL_MSG.getFormattedMessage();
- assertTrue(message.contains(USER), message);
- assertTrue(message.contains(IP_ADDRESS), message);
- assertTrue(message.contains(DummyAction.CREATE_VOLUME.name()), message);
- assertTrue(message.contains(PARAMS.toString()), message);
- assertTrue(message.contains(FAILURE.getStatus()), message);
+ assertThat(message).withFailMessage(message).contains(USER);
+ assertThat(message).withFailMessage(message).contains(IP_ADDRESS);
+
assertThat(message).withFailMessage(message).contains(DummyAction.CREATE_VOLUME.name());
+ assertThat(message).withFailMessage(message).contains(PARAMS.toString());
+ assertThat(message).withFailMessage(message).contains(FAILURE.getStatus());
Review Comment:
With `assertTrue(..., message)`, the second parameter was only passed to get
some useful hint in case of failure. When using `assertThat`, we get such hint
automatically, so we can omit `withFailMessage(message)`.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestRDBTableStore.java:
##########
@@ -692,7 +694,7 @@ public void testDumpAndLoadBasic() throws Exception {
// check dump file exist
assertTrue(dumpFile.exists());
- assertTrue(dumpFile.length() != 0);
+ assertNotEquals(1, dumpFile.length());
Review Comment:
Here the "not equal" value is changed from 0 to 1. Why?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java:
##########
@@ -254,13 +255,12 @@ private void verifyGetProperty(OzoneConfiguration conf,
String format,
// in the response
if (Strings.isNullOrEmpty(propertyName)) {
for (Map.Entry<String, String> entry : TEST_PROPERTIES.entrySet()) {
- assertTrue(result.contains(entry.getKey()) &&
- result.contains(entry.getValue()));
+ assertThat(result).contains(entry.getKey(), entry.getValue());
}
} else {
if (conf.get(propertyName) != null) {
// if property name is not empty and property is found
- assertTrue(result.contains(propertyName));
+ assertThat(result).contains(propertyName);
for (Map.Entry<String, String> entry : TEST_PROPERTIES.entrySet()) {
if (!entry.getKey().equals(propertyName)) {
assertFalse(result.contains(entry.getKey()));
Review Comment:
`assertFalse` assertions can be changed along the same lines:
```
assertThat(result).doesNotContain(entry.getKey());
```
Sorry if that wasn't clear from the task description. I have now updated
the parent issue.
--
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]
