You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hemantk-12 (via GitHub)" <gi...@apache.org> on 2023/12/21 05:09:28 UTC
[PR] HDDS-9170. Deprecate GenericTestUtils#assertExceptionContains and use AssertJ#assertThat [ozone]
hemantk-12 opened a new pull request, #5844:
URL: https://github.com/apache/ozone/pull/5844
## What changes were proposed in this pull request?
This change is to deprecate and remove GenericTestUtils#assertExceptionContains and use AssertJ#assertThat.
AssertJ is already a dependency of ozone and is preferred to use to exception message comparison. More details are in HDDS-9951 and HDDS-9170.
## What is the link to the Apache JIRA
HDDS-9170
## How was this patch tested?
Ran test locally and fork branch.
--
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-9170. Deprecate GenericTestUtils#assertExceptionContains and use AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5844:
URL: https://github.com/apache/ozone/pull/5844#issuecomment-1866163664
> Thanks @hemantk-12 for working on this. Mostly looks good, I only noticed one small problem in `TestSecureOzoneCluster`. Also suggested a small set of related minor improvents in `TestRootedOzoneFileSystem`.
>
> The change is rather large, and has several different improvements. To keep patches smaller and more focused, I suggest the following (for the future):
>
> * When you find an opportunity for refactoring (e.g. converting to `@ParameterizedTest` to reduce duplication), create a follow-up task. Assign it to yourself, if only to avoid other eager contributors jumping on it right away (which can cause conflicts in patches).
>
> * For minor updates, like formatting changes, there are two possible routes:
>
> * make various changes in a limited set of files
> * make the same limited change in many files
>
> Either of these make the review cycle quicker (easier to provide feedback, easier to address comments). I think the first option is better for two reasons: working in parallel with others now, and backporting such changes in the future.
Thanks for the detailed suggestion. I'll keep it in mind and try to raise smaller and cosine patches in future.
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 merged PR #5844:
URL: https://github.com/apache/ozone/pull/5844
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434450082
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
Thanks @hemantk-12 for updating the patch. Two possible issues:
1. `Could not determine the leader node` is a varying part of the exception message. The same part may instead refer to the actual leader, if known. (Check different constructors of `OMNotLeaderException`.) `is not the leader` is common in both cases.
2. `createKeyTest` also needs the same change.
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434433574
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
If the leader and a follower are going down, can the remaining follower step up? Does leader election happens with one node?
If not, this is a flaky test with 66% failure and we should turn off followers only.
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434437100
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
The events in the test are not deterministic in that it may get different exceptions. But it would pass in all branches. I think the problem is that `hasMessageContaining` only checks the exception message, while `assertExceptionContains` checked `e.toString()`, which is slightly different.
`toString()`:
```
org.apache.hadoop.ipc.RemoteException(org.apache.hadoop.ozone.om.exceptions.OMNotLeaderException): OM:omNode-3 is not the leader. Could not determine the leader node.
at org.apache.hadoop.ozone.om.OzoneManager.checkLeaderStatus(OzoneManager.java:4144)
at org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerRatisUtils.checkLeaderStatus(OzoneManagerRatisUtils.java:485)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.internalProcessRequest(OzoneManagerProtocolServerSideTranslatorPB.java:217)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.processRequest(OzoneManagerProtocolServerSideTranslatorPB.java:171)
at org.apache.hadoop.hdds.server.OzoneProtocolMessageDispatcher.processRequest(OzoneProtocolMessageDispatcher.java:89)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.submitRequest(OzoneManagerProtocolServerSideTranslatorPB.java:162)
at org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos$OzoneManagerService$2.callBlockingMethod(OzoneManagerProtocolProtos.java)
at org.apache.hadoop.ipc.ProtobufRpcEngine$Server.processCall(ProtobufRpcEngine.java:484)
at org.apache.hadoop.ipc.ProtobufRpcEngine2$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine2.java:595)
at org.apache.hadoop.ipc.ProtobufRpcEngine2$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine2.java:573)
at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:1227)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1094)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1017)
at java.security.AccessController.doPrivileged(Native Method)
at javax.security.auth.Subject.doAs(Subject.java:422)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1899)
at org.apache.hadoop.ipc.Server$Handler.run(Server.java:3048)
```
`getMessage()`:
```
OM:omNode-3 is not the leader. Could not determine the leader node.
at org.apache.hadoop.ozone.om.OzoneManager.checkLeaderStatus(OzoneManager.java:4144)
at org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerRatisUtils.checkLeaderStatus(OzoneManagerRatisUtils.java:485)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.internalProcessRequest(OzoneManagerProtocolServerSideTranslatorPB.java:217)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.processRequest(OzoneManagerProtocolServerSideTranslatorPB.java:171)
at org.apache.hadoop.hdds.server.OzoneProtocolMessageDispatcher.processRequest(OzoneProtocolMessageDispatcher.java:89)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.submitRequest(OzoneManagerProtocolServerSideTranslatorPB.java:162)
at org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos$OzoneManagerService$2.callBlockingMethod(OzoneManagerProtocolProtos.java)
at org.apache.hadoop.ipc.ProtobufRpcEngine$Server.processCall(ProtobufRpcEngine.java:484)
at org.apache.hadoop.ipc.ProtobufRpcEngine2$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine2.java:595)
at org.apache.hadoop.ipc.ProtobufRpcEngine2$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine2.java:573)
at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:1227)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1094)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1017)
at java.security.AccessController.doPrivileged(Native Method)
at javax.security.auth.Subject.doAs(Subject.java:422)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1899)
at org.apache.hadoop.ipc.Server$Handler.run(Server.java:3048)
```
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434438703
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
So we should either:
```
assertThat(e.toString()).contains("OMNotLeaderException");
```
or:
```
assertThat(e).hasMessageContaining("is not the leader");
```
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434445898
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
Yes, you are right. I just figured that it is an issue with `exception.toString()` and `exception.getMessage()`.
Changed it to:
```
assertThat(e).hasMessageContaining("Could not determine the leader node.");
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
Yes, you are right. I just figured that it is an issue with `exception.toString()` and `exception.getMessage()`.
Changed it to:
```
assertThat(e).hasMessageContaining("Could not determine the leader node.");
```
--
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-9170. Deprecate GenericTestUtils#assertExceptionContains and use AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434001633
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -76,6 +76,7 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
+import org.junit.jupiter.api.Assertions;
Review Comment:
Removed.
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434145365
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
Looks like old and new code work differently, and test may fail with:
```
Expecting:
<"OM:omNode-3 is not the leader. Could not determine the leader node.
at org.apache.hadoop.ozone.om.OzoneManager.checkLeaderStatus(OzoneManager.java:4144)
at org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerRatisUtils.checkLeaderStatus(OzoneManagerRatisUtils.java:485)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.internalProcessRequest(OzoneManagerProtocolServerSideTranslatorPB.java:217)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.processRequest(OzoneManagerProtocolServerSideTranslatorPB.java:171)
at org.apache.hadoop.hdds.server.OzoneProtocolMessageDispatcher.processRequest(OzoneProtocolMessageDispatcher.java:89)
at org.apache.hadoop.ozone.protocolPB.OzoneManagerProtocolServerSideTranslatorPB.submitRequest(OzoneManagerProtocolServerSideTranslatorPB.java:[162](https://github.com/apache/ozone/actions/runs/7288115214/job/19860713339?pr=5844#step:6:163))
at org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos$OzoneManagerService$2.callBlockingMethod(OzoneManagerProtocolProtos.java)
at org.apache.hadoop.ipc.ProtobufRpcEngine$Server.processCall(ProtobufRpcEngine.java:484)
at org.apache.hadoop.ipc.ProtobufRpcEngine2$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine2.java:595)
at org.apache.hadoop.ipc.ProtobufRpcEngine2$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine2.java:573)
at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:1227)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1094)
at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1017)
at java.security.AccessController.doPrivileged(Native Method)
at javax.security.auth.Subject.doAs(Subject.java:422)
at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1899)
at org.apache.hadoop.ipc.Server$Handler.run(Server.java:3048)
">
to contain:
<"OMNotLeaderException">
at org.apache.hadoop.ozone.om.TestOzoneManagerHA.createKeyTest(TestOzoneManagerHA.java:448)
```
https://github.com/apache/ozone/actions/runs/7288115214/job/19860713339?pr=5844#step:6:158
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434478804
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
Fixed.
--
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-9170. Deprecate GenericTestUtils#assertExceptionContains and use AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434001936
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -421,10 +422,11 @@ public void testDeleteCreatesFakeParentDir() throws Exception {
// Creating a child should not add parent keys to the bucket
try {
getKey(parent, true);
- } catch (IOException ex) {
- assertKeyNotFoundException(ex);
+ } catch (OMException ome) {
+ Assertions.assertEquals(KEY_NOT_FOUND, ome.getResult());
Review Comment:
Changed it to static import.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -350,8 +351,8 @@ public void testCreateDoesNotAddParentDirKeys() throws Exception {
// Creating a child should not add parent keys to the bucket
try {
getKey(parent, true);
- } catch (IOException ex) {
- assertKeyNotFoundException(ex);
+ } catch (OMException ome) {
+ Assertions.assertEquals(KEY_NOT_FOUND, ome.getResult());
Review Comment:
Changed it to static import.
--
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-9170. Deprecate GenericTestUtils#assertExceptionContains and use AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434003120
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java:
##########
@@ -188,22 +189,19 @@ public void testCreateContainerFileWithoutReplicaIndex(
cleanup();
}
-
@ContainerLayoutTestInfo.ContainerTest
- public void testIncorrectContainerFile(ContainerLayoutVersion layout)
- throws IOException {
+ public void testIncorrectContainerFile(ContainerLayoutVersion layout) {
setLayoutVersion(layout);
- try {
- String containerFile = "incorrect.container";
- //Get file from resources folder
+ String containerFile = "incorrect.container";
+
+ IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> {
+ // Get file from resource folder
ClassLoader classLoader = getClass().getClassLoader();
File file = new File(classLoader.getResource(containerFile).getFile());
Review Comment:
Done.
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5844:
URL: https://github.com/apache/ozone/pull/5844#issuecomment-1867281496
Thanks @adoroszlai and @duongkame 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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434406561
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
It seems like a flaky test because it passed on fork branch:
* https://github.com/hemantk-12/ozone/actions/runs/7288114820
* https://github.com/hemantk-12/ozone/actions/runs/7279164150
Also previous run passed:
* https://github.com/apache/ozone/actions/runs/7284248479
Are we supposed to stop any two nodes [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithStoppedNodes.java#L115-L116) or follower nodes?
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434450082
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
Thanks @hemantk-12 for updating the patch. Two possible issues:
1. `Could not determine the leader node` is a varying part of the exception message. The same part may instead refer to the actual leader, if known. (Check different constructors of `OMNotLeaderException`.) `is not the leader` is common in both cases.
2. `createKeyTest` needs the same change, too.
--
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-9170. Deprecate GenericTestUtils#assertExceptionContains and use AssertJ#assertThat [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1433902937
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -350,8 +351,8 @@ public void testCreateDoesNotAddParentDirKeys() throws Exception {
// Creating a child should not add parent keys to the bucket
try {
getKey(parent, true);
- } catch (IOException ex) {
- assertKeyNotFoundException(ex);
+ } catch (OMException ome) {
+ Assertions.assertEquals(KEY_NOT_FOUND, ome.getResult());
Review Comment:
`TestRootedOzoneFileSystem` is yet to be migrated (HDDS-9921). We can use existing static import until then:
```suggestion
assertEquals(KEY_NOT_FOUND, ome.getResult());
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerDataYaml.java:
##########
@@ -188,22 +189,19 @@ public void testCreateContainerFileWithoutReplicaIndex(
cleanup();
}
-
@ContainerLayoutTestInfo.ContainerTest
- public void testIncorrectContainerFile(ContainerLayoutVersion layout)
- throws IOException {
+ public void testIncorrectContainerFile(ContainerLayoutVersion layout) {
setLayoutVersion(layout);
- try {
- String containerFile = "incorrect.container";
- //Get file from resources folder
+ String containerFile = "incorrect.container";
+
+ IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> {
+ // Get file from resource folder
ClassLoader classLoader = getClass().getClassLoader();
File file = new File(classLoader.getResource(containerFile).getFile());
Review Comment:
nit: these lines can be moved outside of `assertThrows`, and then it can use a single statement instead of a block.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -76,6 +76,7 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
+import org.junit.jupiter.api.Assertions;
Review Comment:
```suggestion
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java:
##########
@@ -838,21 +840,12 @@ public void testGetSetRevokeS3Secret() throws Exception {
OmTransportFactory.create(conf, ugiNonAdmin, null),
RandomStringUtils.randomAscii(5));
- try {
- omClientNonAdmin.getS3Secret("HADOOP/JOHN");
- // Expected to fail because current ugi isn't an admin
- fail("non-admin getS3Secret didn't fail as intended");
- } catch (IOException ex) {
- GenericTestUtils.assertExceptionContains("USER_MISMATCH", ex);
- }
-
- try {
- omClientNonAdmin.revokeS3Secret("HADOOP/DOE");
- // Expected to fail because current ugi isn't an admin
- fail("non-admin revokeS3Secret didn't fail as intended");
- } catch (IOException ex) {
- GenericTestUtils.assertExceptionContains("USER_MISMATCH", ex);
- }
+ OMException omException = assertThrows(OMException.class,
+ () -> omClientNonAdmin.getS3Secret("HADOOP/JOHN"));
+ assertSame(USER_MISMATCH, omException.getResult());
+ omException = assertThrows(OMException.class,
+ () -> omClientNonAdmin.getS3Secret("HADOOP/DOE"));
Review Comment:
This call was different in the original:
```suggestion
() -> omClientNonAdmin.revokeS3Secret("HADOOP/DOE"));
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -421,10 +422,11 @@ public void testDeleteCreatesFakeParentDir() throws Exception {
// Creating a child should not add parent keys to the bucket
try {
getKey(parent, true);
- } catch (IOException ex) {
- assertKeyNotFoundException(ex);
+ } catch (OMException ome) {
+ Assertions.assertEquals(KEY_NOT_FOUND, ome.getResult());
Review Comment:
```suggestion
assertEquals(KEY_NOT_FOUND, ome.getResult());
```
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434406561
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
It seems like a flaky test because it passed on fork branch:
* https://github.com/hemantk-12/ozone/actions/runs/7288114820
* https://github.com/hemantk-12/ozone/actions/runs/7279164150
Also previous run passed:
* https://github.com/apache/ozone/actions/runs/7284248479
Are we supposed to stop any two nodes [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHAWithStoppedNodes.java#L115-L116) or follower nodes?
--
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-9170. Replace GenericTestUtils#assertExceptionContains with AssertJ#assertThat [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5844:
URL: https://github.com/apache/ozone/pull/5844#discussion_r1434406561
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java:
##########
@@ -336,12 +337,11 @@ protected void createVolumeTest(boolean checkSuccess) throws Exception {
// ConnectException. Otherwise, we would get a RemoteException from the
// last running OM as it would fail to get a quorum.
if (e instanceof RemoteException) {
- GenericTestUtils.assertExceptionContains("OMNotLeaderException", e);
+ assertThat(e).hasMessageContaining("OMNotLeaderException");
Review Comment:
It seems like a flaky test because it passed on fork branch:
* https://github.com/hemantk-12/ozone/actions/runs/7288114820
* https://github.com/hemantk-12/ozone/actions/runs/7279164150
Also previous run passed:
* https://github.com/apache/ozone/actions/runs/7284248479
--
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