[GitHub] [hadoop-ozone] fapifta edited a comment on pull request #1291: HDDS-4062. Non rack aware pipelines should not be created if multiple racks are alive

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

fapifta edited a comment on pull request #1291:
URL: https://github.com/apache/hadoop-ozone/pull/1291#issuecomment-673155132


   Hi @sodonnel,
   
   the changeset looks good to me, I am +1 on committing it, though it is non-binding :)
   
   There is one thing I would change, but as this implementation fits within the rest of the code and kind of consistent with it, we might not need to start the change now or ever, even if I would be happier, but I would like to express my view on testing a bit, and see if I can start a change :) Please if you have anything to add on why you chose this implementation, I would be quite happy to discuss it as I might learn.
   So... as I view, the test method tests 2 things, therefore the name of the method does not tell much about what is being tested, and one needs to carefully read the code and comments to understand what is the requirement being tested.
   Instead of this, if I would have written the tests, I would have written two tests, one named something like testExceptionIsThrownWhenRackAwarePipelineCanNotBeCreated the other named something like test3NodesInSameRackReturnedWhenOnlyOneHealthyRackIsPresent.
   With that, you can easily use a built in ExpectedException JUnit rule to check the thrown exception and assert on the exception message as well in the first test. Yes I am not a fan of LambdaTestUtils, I know it is written as part of the project, but as I see it helps the bad practice of testing more things at the same time, so instead I prefer to @<!-- -->Expect an exception or to use the ExpectedException rule.
   Also if we want to test for the exception message, this way it is pretty much fragile, and I would prefer to have a package private static final string with the exception message being thrown in this case, and checked in the test, though I can live with this approach, as the message likely will not be changed too often.


----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org