-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56964/#review166590
-----------------------------------------------------------
+1 I know this change is good and would be safe commit it as is, however we
have been trying to create UnitTests for any class that we touch that doesn't
already have a UnitTest. As a whole, the project relies too much on end-to-end
tests which take forever to run and tend to be more flaky.
So I still recommend trying to write a UnitTest that at a minimum covers the
method you've modified. My definition of UnitTest here is a very fast test that
uses mocking (Mockito) to mock everything this class method interacts with and
the test does not interact with file system or network. If this method
interacts with file system, then there are generally two choices: test it in an
IntegrationTest or refactor the method to allow you to UnitTest it in isolation
from file system. The test class itself has a JUnit @Category annotation at the
top and the category is either UnitTest.class or IntegrationTest.class in this
case.
For testing an exception, I recommend using AssertJ:
assertThatThrownBy(() ->
target.callMethodThatThrows()).isInstanceOf(IllegalStateException.class).hasMessageContaining("resourceTypeId").hasMessageContaining("resourceName");
You could instead use .hasMessage("ResourceType is missing for resourceTypeId
xxx, resourceName yyy") but that does tend to make for a more brittle test.
- Kirk Lund
On Feb. 23, 2017, 3:03 a.m., Srikanth Manvi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56964/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2017, 3:03 a.m.)
>
>
> Review request for geode and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Enhanced the log statement to include the ResourceType Name.
>
>
> Diffs
> -----
>
>
> geode-core/src/main/java/org/apache/geode/internal/statistics/StatArchiveReader.java
> 9fba511
>
> Diff: https://reviews.apache.org/r/56964/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Srikanth Manvi
>
>
