> 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
>
> Kirk Lund wrote:
> 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
> }
> ```
Hi Kirk,
Thank you for the comments and background around Unit/Integration test options.
I ran ./gradlew build, which was successful, hence I submitted a PR and a
review request. Later on (many hours after I started precheckin) I noticed that
the test you mentioned in StatArchiveWithMissingResourceTypeRegressionTest did
fail. I made changes to the test and pushed a new
commit(https://github.com/apache/geode/pull/406/commits/56c2a7fce00b5a44d50f1cc8374d1da3ab214344#diff-a74ae4844c1a648682d8a0d297976e31R62).
I had a feeling that I was making the test brittle by using
`.hasMessage("ResourceType is missing for resourceTypeId 0, resourceName
statistics1")`;
but at the same I felt it was not too brittle as the gfs file was picked up
from the
org/apache/geode/internal/statistics/StatArchiveWithMissingResourceTypeRegressionTest.gfs
which hasnt changed since creation.
But I agree, your suggestion of using `.hasMessageContaining(...)` , I will
incorporate that and will push a new commit.
If you dont mind I have few questions, apologies if this is not the right
place.
1. I pushed 1 commit and raised a PR and then submitted this review
request(https://reviews.apache.org/r/56964/). Later on realized that the
regression test in StatArchiveWithMissingResourceTypeRegressionTest fails
because of my change. Hence pushed 1 more commit (part of the same PR
#406[smanvi-pivotal wants to merge 2 commits into apache:develop from
smanvi-pivotal:feature/GEODE-2526]) to fix the test and did not create a new
review request. Should every commit(pushed at different times before the PR is
closed) in a PR have its own review request ?
2. For non-committers isn`t opening a PR in github equivalent to opening a
review request in https://reviews.apache.org ? In future should I keep opening
review requests along with PRs or is reviews.apache.org mainly for committers ?
3. I let my "./gradlew precheckin" execute for about 5 hrs and it was still not
complete. Is there anything that I can do to speed it up ? How do regular
contributors/committers push multiple commits/day if each commit needs to be
preceeded by a succesfull precheckin ?
- Srikanth
-----------------------------------------------------------
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
>
>
