You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "adoroszlai (via GitHub)" <gi...@apache.org> on 2024/01/06 08:15:13 UTC
[PR] HDDS-10075. Simplify leftover assertions [ozone]
adoroszlai opened a new pull request, #5935:
URL: https://github.com/apache/ozone/pull/5935
## What changes were proposed in this pull request?
Assertions in the form:
```
assertTrue(<x> == <y>)
assertEquals(null, <x>) or assertTrue(<x> == null)
assertTrue(<x> != null)
assertEquals(<boolean>, <x>)
```
can be simplified to:
```
assertEquals(<x>, <y>)
assertNull(<x>)
assertNotNull(<x>)
assertTrue/assertFalse(<x>)
```
Not only are these simpler, some of them also provide more information in case of failure.
This PR updates such assertions leftover by previous tasks (except `TestSecureOzoneCluster` is done in #5906).
https://issues.apache.org/jira/browse/HDDS-10075
## How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/7427315061
--
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-10075. Simplify leftover assertions [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5935:
URL: https://github.com/apache/ozone/pull/5935#discussion_r1445081816
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/TestSCMNodeManagerMXBean.java:
##########
@@ -98,7 +98,7 @@ public void testNodeCount() throws Exception {
Map<String, Map<String, Integer>> mbeanMap = convertNodeCountToMap(data);
Map<String, Map<String, Integer>> nodeMap =
scm.getScmNodeManager().getNodeCount();
- assertTrue(nodeMap.equals(mbeanMap));
+ assertEquals(nodeMap, mbeanMap);
Review Comment:
I figured that `mbeanMap` is the one being verified here, since its a test for `MXBean`.
--
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-10075. Simplify leftover assertions [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5935:
URL: https://github.com/apache/ozone/pull/5935#discussion_r1445075732
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/TestSCMNodeManagerMXBean.java:
##########
@@ -98,7 +98,7 @@ public void testNodeCount() throws Exception {
Map<String, Map<String, Integer>> mbeanMap = convertNodeCountToMap(data);
Map<String, Map<String, Integer>> nodeMap =
scm.getScmNodeManager().getNodeCount();
- assertTrue(nodeMap.equals(mbeanMap));
+ assertEquals(nodeMap, mbeanMap);
Review Comment:
Same a previous comment.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerRestart.java:
##########
@@ -117,7 +116,7 @@ public void testRestartOMWithVolumeOperation() throws Exception {
objectStore.createVolume(volumeName);
OzoneVolume ozoneVolume = objectStore.getVolume(volumeName);
- assertTrue(ozoneVolume.getName().equals(volumeName));
+ assertEquals(ozoneVolume.getName(), volumeName);
Review Comment:
nit: should be in `assertEquals(expected, actual)` format here and line 130.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1523,13 +1523,10 @@ public void testRefreshPipelineException() throws Exception {
KeyManagerImpl keyManagerImpl =
new KeyManagerImpl(ozoneManager, scmClientMock, conf, metrics);
- try {
- keyManagerImpl.refresh(omKeyInfo);
- fail();
- } catch (OMException omEx) {
- assertEquals(SCM_GET_PIPELINE_EXCEPTION, omEx.getResult());
- assertTrue(omEx.getMessage().equals(errorMessage));
- }
+ OMException omEx = assertThrows(OMException.class,
+ () -> keyManagerImpl.refresh(omKeyInfo));
+ assertEquals(SCM_GET_PIPELINE_EXCEPTION, omEx.getResult());
+ assertEquals(errorMessage, omEx.getMessage());
Review Comment:
nit: should we use `assertThat()` here?
--
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-10075. Simplify leftover assertions [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5935:
URL: https://github.com/apache/ozone/pull/5935
--
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-10075. Simplify leftover assertions [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5935:
URL: https://github.com/apache/ozone/pull/5935#discussion_r1445226423
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1523,13 +1523,10 @@ public void testRefreshPipelineException() throws Exception {
KeyManagerImpl keyManagerImpl =
new KeyManagerImpl(ozoneManager, scmClientMock, conf, metrics);
- try {
- keyManagerImpl.refresh(omKeyInfo);
- fail();
- } catch (OMException omEx) {
- assertEquals(SCM_GET_PIPELINE_EXCEPTION, omEx.getResult());
- assertTrue(omEx.getMessage().equals(errorMessage));
- }
+ OMException omEx = assertThrows(OMException.class,
+ () -> keyManagerImpl.refresh(omKeyInfo));
+ assertEquals(SCM_GET_PIPELINE_EXCEPTION, omEx.getResult());
+ assertEquals(errorMessage, omEx.getMessage());
Review Comment:
Fair point. It doesn't matter much what we use here.
--
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-10075. Simplify leftover assertions [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on PR #5935:
URL: https://github.com/apache/ozone/pull/5935#issuecomment-1881496349
cc @SaketaChalamchala
--
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-10075. Simplify leftover assertions [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5935:
URL: https://github.com/apache/ozone/pull/5935#discussion_r1445084559
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1523,13 +1523,10 @@ public void testRefreshPipelineException() throws Exception {
KeyManagerImpl keyManagerImpl =
new KeyManagerImpl(ozoneManager, scmClientMock, conf, metrics);
- try {
- keyManagerImpl.refresh(omKeyInfo);
- fail();
- } catch (OMException omEx) {
- assertEquals(SCM_GET_PIPELINE_EXCEPTION, omEx.getResult());
- assertTrue(omEx.getMessage().equals(errorMessage));
- }
+ OMException omEx = assertThrows(OMException.class,
+ () -> keyManagerImpl.refresh(omKeyInfo));
+ assertEquals(SCM_GET_PIPELINE_EXCEPTION, omEx.getResult());
+ assertEquals(errorMessage, omEx.getMessage());
Review Comment:
Since it checks equality, not `contains()`, I thought `assertEquals` is good enough.
--
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-10075. Simplify leftover assertions [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5935:
URL: https://github.com/apache/ozone/pull/5935#issuecomment-1881702436
Thanks @hemantk-12 for the review.
--
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-10075. Simplify leftover assertions [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5935:
URL: https://github.com/apache/ozone/pull/5935#discussion_r1445082571
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerRestart.java:
##########
@@ -117,7 +116,7 @@ public void testRestartOMWithVolumeOperation() throws Exception {
objectStore.createVolume(volumeName);
OzoneVolume ozoneVolume = objectStore.getVolume(volumeName);
- assertTrue(ozoneVolume.getName().equals(volumeName));
+ assertEquals(ozoneVolume.getName(), volumeName);
Review Comment:
Thanks. I fixed a few of these before creating the PR, but missed these two.
--
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-10075. Simplify leftover assertions [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5935:
URL: https://github.com/apache/ozone/pull/5935#discussion_r1445081816
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/TestSCMNodeManagerMXBean.java:
##########
@@ -98,7 +98,7 @@ public void testNodeCount() throws Exception {
Map<String, Map<String, Integer>> mbeanMap = convertNodeCountToMap(data);
Map<String, Map<String, Integer>> nodeMap =
scm.getScmNodeManager().getNodeCount();
- assertTrue(nodeMap.equals(mbeanMap));
+ assertEquals(nodeMap, mbeanMap);
Review Comment:
I figured that `mbeanMap` is the one being verified here, since it's a test for `MXBean`.
--
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