> On Feb. 23, 2017, 10:18 p.m., Kirk Lund wrote:
> > +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 wrote:
> In this case, you could probably just modify
> StatArchiveWithMissingResourceTypeRegressionTest
Also, the travis CI that's hooked up to the github project for geode is
executing test but it doesn't run integrationTest or distributedTest. If you
run integrationTest, I think you'll see
StatArchiveWithMissingResourceTypeRegressionTest fail due to your change.
You'll want to update the assertion in this test:
```java
@Test // fixed GEODE-2013
public void throwsIllegalStateExceptionWithMessage() throws Exception {
assertThatThrownBy(() -> new StatArchiveReader(new File[]
{this.archiveFile}, null, true))
.isExactlyInstanceOf(IllegalStateException.class) // was
NullPointerException
.hasMessage("ResourceType is missing for resourceTypeId 0"); // was null
}
```
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56964/#review166590
-----------------------------------------------------------
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
>
>
