You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "wzhallright (via GitHub)" <gi...@apache.org> on 2024/01/10 03:41:11 UTC
[PR] HDDS-10092. improve assertTrue assertions in client integration tests [ozone]
wzhallright opened a new pull request, #5969:
URL: https://github.com/apache/ozone/pull/5969
## What changes were proposed in this pull request?
Improve assertTrue assertions in hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client
(see parent task [9551](https://issues.apache.org/jira/browse/HDDS-9551) for details)
## What is the link to the Apache JIRA
[HDDS-10092](https://issues.apache.org/jira/browse/HDDS-10092)
## CI
https://github.com/wzhallright/ozone/actions/runs/7470034758
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10092. improve assertTrue assertions in client integration tests [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5969:
URL: https://github.com/apache/ozone/pull/5969
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10092. improve assertTrue assertions in client integration tests [ozone]
Posted by "wzhallright (via GitHub)" <gi...@apache.org>.
wzhallright commented on PR #5969:
URL: https://github.com/apache/ozone/pull/5969#issuecomment-1884692099
@adoroszlai Thanks for your detailed comments and suggestions. I have fixed the comment. PTAL..
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10092. improve assertTrue assertions in client integration tests [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10092. improve assertTrue assertions in client integration tests [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5969:
URL: https://github.com/apache/ozone/pull/5969#issuecomment-1885792001
Thanks @wzhallright for the patch.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org