pkuwm commented on a change in pull request #976:
URL: https://github.com/apache/helix/pull/976#discussion_r417014257
##########
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:
I understand your point. And I agree we shouldn’t do redundant code. But
in this specific case here, since compression is such a critical part, I would
prefer to make it bold. If there is anything change in the logic and triggers
the test failed, the message should tell the reason. And one more reason I use
assert.fail is to avoid that if someone changes the test and catch the
exception Without failing the test , the exception thrown from here may not
fail the test. This is what I would like to prevent.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
