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/02 06:02:56 UTC
[PR] HDDS-10033. Improve assertTrue assertions in ozone-manager [ozone]
wzhallright opened a new pull request, #5901:
URL: https://github.com/apache/ozone/pull/5901
## What changes were proposed in this pull request?
Improve assertTrue/assertFalse assertions in hadoop-ozone/ozone-manager
(see parent task [HDDS-9951](https://issues.apache.org/jira/browse/HDDS-9951) for details)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10033
## CI
https://github.com/wzhallright/ozone/actions/runs/7382711143
--
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-10033. Improve assertTrue assertions in ozone-manager [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5901:
URL: https://github.com/apache/ozone/pull/5901#discussion_r1439215584
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -633,16 +634,16 @@ public void testCreateSnapshotIdempotent() throws Exception {
// Create first checkpoint for the snapshot checkpoint
OmSnapshotManager.createOmSnapshotCheckpoint(om.getMetadataManager(),
first);
- assertFalse(logCapturer.getOutput().contains(
- "for snapshot " + first.getName() + " already exists."));
+ assertThat(logCapturer.getOutput()).contains(
+ "for snapshot " + first.getName() + " already exists.");
Review Comment:
This one was `assertFalse`:
```suggestion
assertThat(logCapturer.getOutput()).doesNotContain(
"for snapshot " + first.getName() + " already exists.");
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java:
##########
@@ -806,7 +807,7 @@ private void checkNotAValidPath(String keyName) {
omKeyCreateRequest.preExecute(ozoneManager);
fail("checkNotAValidPath failed for path" + keyName);
} catch (IOException ex) {
- assertTrue(ex instanceof OMException);
+ assertInstanceOf(OMException.class, ex);
OMException omException = (OMException) ex;
Review Comment:
nit: cast can be removed
```suggestion
OMException omException = assertInstanceOf(OMException.class, ex);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java:
##########
@@ -282,7 +283,7 @@ public void testFetchSecretForRevokedUser() throws IOException {
ozoneManager, 2);
// Verify that the revoke operation was successful.
- assertTrue(omRevokeResponse instanceof S3RevokeSecretResponse);
+ assertInstanceOf(S3RevokeSecretResponse.class, omRevokeResponse);
S3RevokeSecretResponse s3RevokeSecretResponse =
(S3RevokeSecretResponse) omRevokeResponse;
Review Comment:
nit: cast can be removed
```suggestion
S3RevokeSecretResponse s3RevokeSecretResponse =
assertInstanceOf(S3RevokeSecretResponse.class, omRevokeResponse);
```
(Also in other test cases in this class.)
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -1174,7 +1175,7 @@ public void testListSnapshotDiffJobs(String jobStatus,
// there should be a response.
// Otherwise, response list should be empty.
if (containsJob) {
- assertTrue(jobList.contains(diffJob));
+ assertThat(jobList).contains(diffJob);
} else {
assertTrue(jobList.isEmpty());
Review Comment:
nit: this can be replaced, too. This way test failure shows list contents if it is found not empty.
```suggestion
assertThat(jobList).isEmpty();
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerUnit.java:
##########
@@ -515,9 +515,9 @@ public void testLookupFileWithDnFailure() throws IOException {
.getBlockID().getLocalID());
assertEquals(pipelineTwo.getId(),
newBlockLocation.getPipeline().getId());
- assertTrue(newBlockLocation.getPipeline().getNodes().contains(dnFour));
- assertTrue(newBlockLocation.getPipeline().getNodes().contains(dnFive));
- assertTrue(newBlockLocation.getPipeline().getNodes().contains(dnSix));
+ assertThat(newBlockLocation.getPipeline().getNodes()).contains(dnFour);
+ assertThat(newBlockLocation.getPipeline().getNodes()).contains(dnFive);
+ assertThat(newBlockLocation.getPipeline().getNodes()).contains(dnSix);
Review Comment:
nit: we can check multiple items in a single call
```suggestion
assertThat(newBlockLocation.getPipeline().getNodes())
.contains(dnFour, dnFive, dnSix);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerRatisServer.java:
##########
@@ -197,10 +197,9 @@ public void testIsReadOnlyCapturesAllCmdTypeEnums() throws Exception {
.setClientId(clientId)
.build();
OmUtils.isReadOnly(request);
- assertFalse(
- logCapturer.getOutput().contains("CmdType " + cmdtype + " is not " +
- "categorized as readOnly or not."),
- cmdtype + " is not categorized in OmUtils#isReadyOnly");
+ assertThat(logCapturer.getOutput())
+ .withFailMessage(cmdtype + " is not categorized in OmUtils#isReadyOnly")
+ .doesNotContain("CmdType " + cmdtype + " is not " + "categorized as readOnly or not.");
Review Comment:
nit:
```suggestion
.doesNotContain("CmdType " + cmdtype + " is not categorized as readOnly or not.");
```
--
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-10033. Improve assertTrue assertions in ozone-manager [ozone]
Posted by "wzhallright (via GitHub)" <gi...@apache.org>.
wzhallright commented on PR #5901:
URL: https://github.com/apache/ozone/pull/5901#issuecomment-1873762812
@adoroszlai Thanks for your review. Comments have been fixed, please check.
--
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-10033. Improve assertTrue assertions in ozone-manager [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5901:
URL: https://github.com/apache/ozone/pull/5901
--
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