[GitHub] [helix] jiajunwang commented on a change in pull request #976: Fix incorrect exception type and confusing assertion messages

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #976:
URL: https://github.com/apache/helix/pull/976#discussion_r416977623



##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZNRecordSizeLimit.java
##########
@@ -86,8 +89,8 @@ public void testZNRecordSizeLimitUseZNRecordSerializer() {
     _gZkClient.createPersistent(path2, true);
     try {
       _gZkClient.writeData(path2, largeRecord);
-    } catch (HelixException e) {
-      Assert.fail("Should not fail because data size is larger than 1M since compression applied");
+    } catch (ZkMarshallingError e) {

Review comment:
       What is this test log testing?
   
   If the expectation is no error, then we don't care what is the exception. If the operation fails, just throw it. If we use Assert.fail(), we get a message, but we are actually not sure the detail of the exception, because we never include it in the assert fail message.
   If the expectation is we have an error, then the assert in the catch block will fail the test.
   
   Can we make it straight?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org