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 2023/12/18 17:07:32 UTC
[PR] HDDS-9953. Simplify assertions in hadoop-hdds [ozone]
adoroszlai opened a new pull request, #5817:
URL: https://github.com/apache/ozone/pull/5817
## 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 in `hadoop-hdds`.
https://issues.apache.org/jira/browse/HDDS-9953
## How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/7250200952
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5817:
URL: https://github.com/apache/ozone/pull/5817#discussion_r1430910905
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -810,7 +811,7 @@ public void testPutBlockWithInvalidBCSId(ContainerTestVersionInfo versionInfo)
getBlock(container, blockID1);
Assertions.fail("Expected exception not thrown");
} catch (StorageContainerException sce) {
- Assertions.assertTrue(sce.getResult() == UNKNOWN_BCSID);
+ assertSame(UNKNOWN_BCSID, sce.getResult());
Review Comment:
`assertSame` for objects verifies reference equality, like `assertTrue( == )`.
Equality of enum values can be checked both with `equals` and `==`, since each value is a singleton.
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5817:
URL: https://github.com/apache/ozone/pull/5817#discussion_r1430449219
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -127,31 +128,26 @@ public void testStartBalancerStop() throws Exception {
startBalancer(balancerConfiguration);
try {
containerBalancer.startBalancer(balancerConfiguration);
- Assertions.assertTrue(false,
- "Exception should be thrown when startBalancer again");
+ Assertions.fail("Exception should be thrown when startBalancer again");
Review Comment:
I think we should simplify this and use assertThrow.
I looked at sub tasks of HDDS-9952 but didn't see any sub tasks. May be create one if you don't have already.
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #5817:
URL: https://github.com/apache/ozone/pull/5817#discussion_r1430727756
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -821,7 +822,7 @@ public void testPutBlockWithInvalidBCSId(ContainerTestVersionInfo versionInfo)
getBlock(container, blockID1);
Assertions.fail("Expected exception not thrown");
} catch (StorageContainerException sce) {
- Assertions.assertTrue(sce.getResult() == BCSID_MISMATCH);
+ assertSame(BCSID_MISMATCH, sce.getResult());
Review Comment:
Same as above
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #5817:
URL: https://github.com/apache/ozone/pull/5817#discussion_r1430727584
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java:
##########
@@ -810,7 +811,7 @@ public void testPutBlockWithInvalidBCSId(ContainerTestVersionInfo versionInfo)
getBlock(container, blockID1);
Assertions.fail("Expected exception not thrown");
} catch (StorageContainerException sce) {
- Assertions.assertTrue(sce.getResult() == UNKNOWN_BCSID);
+ assertSame(UNKNOWN_BCSID, sce.getResult());
Review Comment:
Should this be `assertEquals`?
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5817:
URL: https://github.com/apache/ozone/pull/5817#discussion_r1430462122
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -127,31 +128,26 @@ public void testStartBalancerStop() throws Exception {
startBalancer(balancerConfiguration);
try {
containerBalancer.startBalancer(balancerConfiguration);
- Assertions.assertTrue(false,
- "Exception should be thrown when startBalancer again");
+ Assertions.fail("Exception should be thrown when startBalancer again");
Review Comment:
Thanks for the suggestion. I also planned to create another task for `assertThrows`, now filed HDDS-9956.
Creating these as top-level issues so that we can create sub-tasks for specific modules if needed (to avoid too large patches).
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5817:
URL: https://github.com/apache/ozone/pull/5817#discussion_r1430449219
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -127,31 +128,26 @@ public void testStartBalancerStop() throws Exception {
startBalancer(balancerConfiguration);
try {
containerBalancer.startBalancer(balancerConfiguration);
- Assertions.assertTrue(false,
- "Exception should be thrown when startBalancer again");
+ Assertions.fail("Exception should be thrown when startBalancer again");
Review Comment:
I think we should simplify this and use assertThrow.
I looked at sub tasks of HDDS-9952 but didn't see any sub tasks for it. May be create one if you don't have already.
--
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-9953. Simplify assertions in hadoop-hdds [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5817:
URL: https://github.com/apache/ozone/pull/5817
--
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