adoroszlai commented on code in PR #5969:
URL: https://github.com/apache/ozone/pull/5969#discussion_r1447183471
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockOutputStreamWithFailures.java:
##########
@@ -532,8 +544,8 @@ private void
testWatchForCommitWithSingleNodeRatis(OzoneClient client)
assertInstanceOf(ContainerNotOpenException.class,
checkForException(blockOutputStream.getIoException()));
- assertTrue(checkForException(blockOutputStream
- .getIoException()) instanceof ContainerNotOpenException);
+ assertInstanceOf(ContainerNotOpenException.class,
+ checkForException(blockOutputStream.getIoException()));
Review Comment:
Same here, this seems to be duplicate.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockOutputStreamWithFailures.java:
##########
@@ -454,8 +463,8 @@ private void testExceptionDuringClose(OzoneClient client)
throws Exception {
assertInstanceOf(ContainerNotOpenException.class,
checkForException(blockOutputStream.getIoException()));
- assertTrue(checkForException(blockOutputStream
- .getIoException()) instanceof ContainerNotOpenException);
+ assertInstanceOf(ContainerNotOpenException.class,
+ checkForException(blockOutputStream.getIoException()));
Review Comment:
This seems to be the same as the existing `assertInstanceOf` above. I think
we can keep one and remove the other.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3121,7 +3123,7 @@ void testCommitPartAfterCompleteUpload() throws Exception
{
ozoneOutputStream.close();
fail("testCommitPartAfterCompleteUpload failed");
} catch (IOException ex) {
- assertTrue(ex instanceof OMException);
+ assertInstanceOf(OMException.class, ex);
assertEquals(NO_SUCH_MULTIPART_UPLOAD_ERROR,
((OMException) ex).getResult());
Review Comment:
Same here, store result of `assertInstanceOf` in local var.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3620,14 +3629,15 @@ private void validateOzoneAccessAcl(OzoneObj ozObj)
throws IOException {
&& acl.getType().equals(newAcl.getType()))
.findFirst();
assertTrue(nonReadAcl.isPresent(), "New acl expected but not found.");
- assertFalse(nonReadAcl.get().getAclList().contains(ACLType.READ_ACL),
- "READ_ACL should not exist in current acls:" + nonReadAcl.get());
+ assertThat(nonReadAcl.get().getAclList())
+ .withFailMessage("READ_ACL should not exist in current acls:" +
nonReadAcl.get())
+ .doesNotContain(ACLType.READ_ACL);
} else {
fail("Default acl should not be empty.");
}
List<OzoneAcl> keyAcls = store.getAcl(ozObj);
- expectedAcls.forEach(a -> assertTrue(keyAcls.contains(a)));
+ expectedAcls.forEach(a -> assertThat(keyAcls).contains(a));
Review Comment:
I think this can be simplified to
```java
assertThat(keyAcls).containsAll(expectedAcls);
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3401,20 +3403,24 @@ private void validateDefaultAcls(OzoneObj parentObj,
OzoneObj childObj,
}
}
List<OzoneAcl> acls = store.getAcl(parentObj);
- assertTrue(acls.contains(defaultUserAcl),
- "Current acls: " + StringUtils.join(",", acls) +
- " inheritedUserAcl: " + inheritedUserAcl);
- assertTrue(acls.contains(defaultGroupAcl),
- "Current acls: " + StringUtils.join(",", acls) +
- " inheritedGroupAcl: " + inheritedGroupAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls: " + StringUtils.join(",", acls) +
+ " inheritedUserAcl: " + inheritedUserAcl)
+ .contains(defaultUserAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls: " + StringUtils.join(",", acls) +
+ " inheritedGroupAcl: " + inheritedGroupAcl)
+ .contains(defaultGroupAcl);
acls = store.getAcl(childObj);
- assertTrue(acls.contains(inheritedUserAcl),
- "Current acls:" + StringUtils.join(",", acls) +
- " inheritedUserAcl:" + inheritedUserAcl);
- assertTrue(acls.contains(inheritedGroupAcl),
- "Current acls:" + StringUtils.join(",", acls) +
- " inheritedGroupAcl:" + inheritedGroupAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls:" + StringUtils.join(",", acls) +
+ " inheritedUserAcl:" + inheritedUserAcl)
+ .contains(inheritedUserAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls:" + StringUtils.join(",", acls) +
+ " inheritedGroupAcl:" + inheritedGroupAcl)
Review Comment:
nit: custom fail message can be omitted.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3607,8 +3615,9 @@ private void validateOzoneAccessAcl(OzoneObj ozObj)
throws IOException {
&& acl.getType().equals(newAcl.getType()))
.findFirst();
assertTrue(readAcl.isPresent(), "New acl expected but not found.");
- assertTrue(readAcl.get().getAclList().contains(ACLType.READ_ACL),
- "READ_ACL should exist in current acls:" + readAcl.get());
+ assertThat(readAcl.get().getAclList())
+ .withFailMessage("READ_ACL should exist in current acls:" +
readAcl.get())
Review Comment:
nit: custom fail message can be omitted.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -2846,7 +2848,7 @@ public void testMultipartUploadWithACL() throws Exception
{
try (OzoneInputStream ignored = bucket2.readKey(keyName)) {
fail("User without permission should fail");
} catch (Exception e) {
- assertTrue(e instanceof OMException);
+ assertInstanceOf(OMException.class, e);
assertEquals(ResultCodes.PERMISSION_DENIED,
((OMException) e).getResult());
Review Comment:
Same here, store result of `assertInstanceOf` in local var.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestDeleteWithInAdequateDN.java:
##########
@@ -325,9 +326,8 @@ public void testDeleteKeyWithInAdequateDN() throws
Exception {
}
fail("Expected exception is not thrown");
} catch (IOException ioe) {
- assertTrue(ioe instanceof StorageContainerException);
- assertSame(((StorageContainerException) ioe).getResult(),
- ContainerProtos.Result.UNABLE_TO_FIND_CHUNK);
+ StorageContainerException e =
assertInstanceOf(StorageContainerException.class, ioe);
+ assertSame(e.getResult(), ContainerProtos.Result.UNABLE_TO_FIND_CHUNK);
Review Comment:
Let's swap the arguments to match `assertSame(expected, actual)`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3058,7 +3060,7 @@ void testAbortUploadFailWithInProgressPartUpload() throws
Exception {
ozoneOutputStream.close();
fail("testAbortUploadFailWithInProgressPartUpload failed");
} catch (IOException ex) {
- assertTrue(ex instanceof OMException);
+ assertInstanceOf(OMException.class, ex);
assertEquals(NO_SUCH_MULTIPART_UPLOAD_ERROR,
((OMException) ex).getResult());
Review Comment:
Same here, store result of `assertInstanceOf` in local var.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3401,20 +3403,24 @@ private void validateDefaultAcls(OzoneObj parentObj,
OzoneObj childObj,
}
}
List<OzoneAcl> acls = store.getAcl(parentObj);
- assertTrue(acls.contains(defaultUserAcl),
- "Current acls: " + StringUtils.join(",", acls) +
- " inheritedUserAcl: " + inheritedUserAcl);
- assertTrue(acls.contains(defaultGroupAcl),
- "Current acls: " + StringUtils.join(",", acls) +
- " inheritedGroupAcl: " + inheritedGroupAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls: " + StringUtils.join(",", acls) +
+ " inheritedUserAcl: " + inheritedUserAcl)
+ .contains(defaultUserAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls: " + StringUtils.join(",", acls) +
+ " inheritedGroupAcl: " + inheritedGroupAcl)
+ .contains(defaultGroupAcl);
acls = store.getAcl(childObj);
- assertTrue(acls.contains(inheritedUserAcl),
- "Current acls:" + StringUtils.join(",", acls) +
- " inheritedUserAcl:" + inheritedUserAcl);
- assertTrue(acls.contains(inheritedGroupAcl),
- "Current acls:" + StringUtils.join(",", acls) +
- " inheritedGroupAcl:" + inheritedGroupAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls:" + StringUtils.join(",", acls) +
+ " inheritedUserAcl:" + inheritedUserAcl)
Review Comment:
nit: custom fail message can be omitted.
```suggestion
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3620,14 +3629,15 @@ private void validateOzoneAccessAcl(OzoneObj ozObj)
throws IOException {
&& acl.getType().equals(newAcl.getType()))
.findFirst();
assertTrue(nonReadAcl.isPresent(), "New acl expected but not found.");
- assertFalse(nonReadAcl.get().getAclList().contains(ACLType.READ_ACL),
- "READ_ACL should not exist in current acls:" + nonReadAcl.get());
+ assertThat(nonReadAcl.get().getAclList())
+ .withFailMessage("READ_ACL should not exist in current acls:" +
nonReadAcl.get())
Review Comment:
nit: custom fail message can be omitted.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -2811,7 +2813,7 @@ public void testMultipartUploadWithACL() throws Exception
{
initiateMultipartUpload(bucket2, keyName2, anyReplication());
fail("User without permission should fail");
} catch (Exception e) {
- assertTrue(e instanceof OMException);
+ assertInstanceOf(OMException.class, e);
assertEquals(ResultCodes.PERMISSION_DENIED,
((OMException) e).getResult());
Review Comment:
We can avoid the need to cast here:
```java
OMException ome = assertInstanceOf(OMException.class, e);
assertEquals(ResultCodes.PERMISSION_DENIED, ome.getResult());
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3468,10 +3474,12 @@ public void testNativeAclsForKey() throws Exception {
// Prefix should inherit DEFAULT acl from bucket.
List<OzoneAcl> acls = store.getAcl(prefixObj);
- assertTrue(acls.contains(inheritedUserAcl),
- "Current acls:" + StringUtils.join(",", acls));
- assertTrue(acls.contains(inheritedGroupAcl),
- "Current acls:" + StringUtils.join(",", acls));
+ assertThat(acls)
+ .withFailMessage("Current acls:" + StringUtils.join(",", acls))
+ .contains(inheritedUserAcl);
+ assertThat(acls)
+ .withFailMessage("Current acls:" + StringUtils.join(",", acls))
Review Comment:
nit: custom fail message can be omitted.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3468,10 +3474,12 @@ public void testNativeAclsForKey() throws Exception {
// Prefix should inherit DEFAULT acl from bucket.
List<OzoneAcl> acls = store.getAcl(prefixObj);
- assertTrue(acls.contains(inheritedUserAcl),
- "Current acls:" + StringUtils.join(",", acls));
- assertTrue(acls.contains(inheritedGroupAcl),
- "Current acls:" + StringUtils.join(",", acls));
+ assertThat(acls)
+ .withFailMessage("Current acls:" + StringUtils.join(",", acls))
Review Comment:
nit: custom fail message can be omitted.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -3646,7 +3656,7 @@ private void validateOzoneAccessAcl(OzoneObj ozObj)
throws IOException {
newAcls = store.getAcl(ozObj);
assertEquals(expectedAcls.size(), newAcls.size());
List<OzoneAcl> finalNewAcls = newAcls;
- expectedAcls.forEach(a -> assertTrue(finalNewAcls.contains(a)));
+ expectedAcls.forEach(a -> assertThat(finalNewAcls).contains(a));
Review Comment:
```suggestion
assertThat(finalNewAcls).containsAll(finalNewAcls);
```
--
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]
