adoroszlai commented on code in PR #5850:
URL: https://github.com/apache/ozone/pull/5850#discussion_r1434043558
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestAddRemoveOzoneManager.java:
##########
@@ -197,9 +199,8 @@ public void testBootstrap() throws Exception {
GenericTestUtils.waitFor(() -> cluster.getOMLeader() != null, 500, 30000);
OzoneManager omLeader = cluster.getOMLeader();
- assertTrue(newOMNodeIds.contains(omLeader.getOMNodeId()),
- "New Bootstrapped OM not elected Leader even though" +
- " other OMs are down");
+ assertThat(newOMNodeIds).withFailMessage("New Bootstrapped OM not elected
Leader even though" +
+ " other OMs are down").contains(omLeader.getOMNodeId());
Review Comment:
```suggestion
assertThat(newOMNodeIds)
.withFailMessage("New Bootstrapped OM not elected Leader even though
other OMs are down")
.contains(omLeader.getOMNodeId());
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileInterfaces.java:
##########
@@ -259,12 +261,9 @@ public void testOzFsReadWrite() throws IOException {
o3fs.pathToKey(path));
// verify prefix directories and the file, do not already exist
- assertTrue(
- metadataManager.getKeyTable(getBucketLayout()).get(lev1key) == null);
- assertTrue(
- metadataManager.getKeyTable(getBucketLayout()).get(lev2key) == null);
- assertTrue(
- metadataManager.getKeyTable(getBucketLayout()).get(fileKey) == null);
+ assertNull(metadataManager.getKeyTable(getBucketLayout()).get(lev1key));
+ assertNull(metadataManager.getKeyTable(getBucketLayout()).get(lev2key));
+ assertNull(metadataManager.getKeyTable(getBucketLayout()).get(fileKey));
Review Comment:
https://github.com/apache/ozone/pull/5846#discussion_r1433679973 applies to
this PR, too. Sorry we have to wait for that one. :)
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestAddRemoveOzoneManager.java:
##########
@@ -131,10 +132,10 @@ private void assertNewOMExistsInPeerList(String nodeId)
throws Exception {
+ " not present in Peer list of OM " + om.getOMNodeId());
assertTrue(om.getOmRatisServer().doesPeerExist(nodeId), "New OM node " +
nodeId
+ " not present in Peer list of OM " + om.getOMNodeId() + "
RatisServer");
- assertTrue(
- om.getOmRatisServer().getCurrentPeersFromRaftConf().contains(nodeId),
- "New OM node " + nodeId + " not present in " + "OM "
- + om.getOMNodeId() + "RatisServer's RaftConf");
+ assertThat(
+ om.getOmRatisServer().getCurrentPeersFromRaftConf())
+ .withFailMessage("New OM node " + nodeId + " not present in " +
"OM "
+ + om.getOMNodeId() + "RatisServer's RaftConf").contains(nodeId);
Review Comment:
Nit: please improve formatting a bit. If we need to break the long line,
let's break it at points that help make it easier to understand what is being
asserted. Middle of `"long string"` is not such a point, but start of a new
method call is.
```suggestion
assertThat(om.getOmRatisServer().getCurrentPeersFromRaftConf())
.withFailMessage("New OM node " + nodeId + " not present in " +
om.getOMNodeId() + "'s RaftConf")
.contains(nodeId);
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java:
##########
@@ -44,6 +44,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
+import static org.assertj.core.api.Assertions.assertThat;
Review Comment:
Heads up: `assertThat` is being added to `TestOzoneManagerConfiguration` and
`TestOzoneManagerHAWithAllRunning` in #5844, too. We'll need another round to
resolve conflicts when that one is merged.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java:
##########
@@ -324,8 +324,8 @@ public void testKeyOps() throws Exception {
writeClient.commitKey(keyArgs, keySession.getId());
} catch (Exception e) {
//Expected Failure in preExecute due to not enough datanode
- assertTrue(e.getMessage().contains("No enough datanodes to choose"),
- e::getMessage);
+ assertThat(e.getMessage()).withFailMessage(e.getMessage())
+ .contains("No enough datanodes to choose");
Review Comment:
nit: custom fail message is not needed
```suggestion
assertThat(e.getMessage()).contains("No enough datanodes to choose");
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -847,9 +848,8 @@ public void testLookupKeyWithLocation() throws IOException {
// lookup key, random node as client
OmKeyInfo key4 = keyManager.lookupKey(keyArgs, resolvedBucket(),
"/d=default-drack/127.0.0.1");
- assertTrue(
- keyPipeline.getNodes().containsAll(key4.getLatestVersionLocations()
- .getLocationList().get(0).getPipeline().getNodesInOrder()));
+
assertThat(keyPipeline.getNodes()).containsAll(key4.getLatestVersionLocations()
+ .getLocationList().get(0).getPipeline().getNodesInOrder());
Review Comment:
```suggestion
assertThat(keyPipeline.getNodes())
.containsAll(key4.getLatestVersionLocations()
.getLocationList().get(0).getPipeline().getNodesInOrder());
```
--
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]
