You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@geode.apache.org by Darrel Schneider <ds...@pivotal.io> on 2016/05/24 16:22:50 UTC

Re: Review Request 47748: change isEmpty to use size

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47748/
-----------------------------------------------------------

(Updated May 24, 2016, 9:22 a.m.)


Review request for geode, Scott Jewell and Ken Howe.


Bugs: GEODE-1424
    https://issues.apache.org/jira/browse/GEODE-1424


Repository: geode


Description
-------

change isEmpty to use size


Diffs
-----

  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalDataSet.java d8e48e13ea019b19e508ad9987c2789918a14189 
  geode-core/src/test/java/com/gemstone/gemfire/internal/cache/LocalDataSetTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/47748/diff/


Testing
-------

precheckin


Thanks,

Darrel Schneider

Re: Review Request 47748: change isEmpty to use size

Posted by Darrel Schneider <ds...@pivotal.io>.


> On May 24, 2016, 10:18 a.m., Ken Howe wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/LocalDataSetTest.java, line 37
> > <https://reviews.apache.org/r/47748/diff/1/?file=1392002#file1392002line37>
> >
> >     Is it necesary to mock pr.isEmpty()? With the change in LocalDataSet.isEmpty I would think that calling PartionedRegion.isEmpty should cause test failures in both of these new tests. So, something like this:
> >     
> >     verify(pr, never()).isEmpty()

I started out with wanting the unit test to fail with the old implementation which only used pr.isEmpty() to determine if the localDataSet was empty.
Then I wanted to confirm that the new implementation would honor what pr.entryCount(Set) returns even if it differed from pr.isEmpty().

You are correct that I could have instead said "verify that localDataSet.isEmpty never calls pr.isEmpty". Its not clear to me if that is better.
I prefer the current unit test because it does not say "you can never call pr.isEmpty". It just says "you answer needs to be consistent with pr.entryCount(Set) and not pr.isEmpty".


- Darrel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47748/#review134604
-----------------------------------------------------------


On May 24, 2016, 9:22 a.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47748/
> -----------------------------------------------------------
> 
> (Updated May 24, 2016, 9:22 a.m.)
> 
> 
> Review request for geode, Scott Jewell and Ken Howe.
> 
> 
> Bugs: GEODE-1424
>     https://issues.apache.org/jira/browse/GEODE-1424
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> change isEmpty to use size
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalDataSet.java d8e48e13ea019b19e508ad9987c2789918a14189 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/LocalDataSetTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47748/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 47748: change isEmpty to use size

Posted by Ken Howe <kh...@pivotal.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47748/#review134604
-----------------------------------------------------------




geode-core/src/test/java/com/gemstone/gemfire/internal/cache/LocalDataSetTest.java (line 37)
<https://reviews.apache.org/r/47748/#comment199450>

    Is it necesary to mock pr.isEmpty()? With the change in LocalDataSet.isEmpty I would think that calling PartionedRegion.isEmpty should cause test failures in both of these new tests. So, something like this:
    
    verify(pr, never()).isEmpty()


- Ken Howe


On May 24, 2016, 4:22 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47748/
> -----------------------------------------------------------
> 
> (Updated May 24, 2016, 4:22 p.m.)
> 
> 
> Review request for geode, Scott Jewell and Ken Howe.
> 
> 
> Bugs: GEODE-1424
>     https://issues.apache.org/jira/browse/GEODE-1424
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> change isEmpty to use size
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalDataSet.java d8e48e13ea019b19e508ad9987c2789918a14189 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/LocalDataSetTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47748/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 47748: change isEmpty to use size

Posted by Ken Howe <kh...@pivotal.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47748/#review134612
-----------------------------------------------------------


Ship it!




I understand the reasoning behind the test implementation and seems acceptable to me.

- Ken Howe


On May 24, 2016, 4:22 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47748/
> -----------------------------------------------------------
> 
> (Updated May 24, 2016, 4:22 p.m.)
> 
> 
> Review request for geode, Scott Jewell and Ken Howe.
> 
> 
> Bugs: GEODE-1424
>     https://issues.apache.org/jira/browse/GEODE-1424
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> change isEmpty to use size
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalDataSet.java d8e48e13ea019b19e508ad9987c2789918a14189 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/LocalDataSetTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47748/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>