> 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
> }
> ```
>
> Srikanth Manvi wrote:
> 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 ?
1. For a PR, I would use a feature/GEODE-nnn branch and use "git rebase -i
origin/develop" after doing a git fetch to collapse multiple commits into 1
commit. Since it's your own branch you can then do a "git push --force" and
this will automatically update the PR for you. It'll rerun travis automatically
too.
2. I would recommend doing just a github PR and we'll do the review there on
github. I wouldn't use reviews.apache.org unless you're a committer and will be
pushing your own changes to the apache git repo.
3. Yeah, the Geode Nightly Build runs precheckin and it takes around 12 hours.
Jens setup a CI infrastructure internal at Pivotal which runs test,
integrationTest, distributedTest and flakyTest all in parallel. I think he also
has distributedTest running its dunit tests in parallel as well. The whole
process takes 3-4 hours and uses docker. I recommend asking about this on dev
list -- maybe you can set up something similar.
- 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
>
>
