----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47855/#review135061 -----------------------------------------------------------
Ship it! Discussed an alternate implementation for adding TypeRegistry for unit testing: Refactor, leaving the original method signatures for basicCreate(), createWithAsyncEventListeners(), and GemFireCachecImpl constructor, and add new methods with the new TypeRegistry parameter. Advantage would be leaving existing calls in product code as is, and use the TypeRegistry parameter (which is effectively a test hook) in the unit test. That said, all of the changes in GemFireCachImpl are in private methods/contrustors, so the signature change is not exposed outside of the class. Code as submitted for review looks good to go. - Ken Howe On May 25, 2016, 9:42 p.m., Darrel Schneider wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47855/ > ----------------------------------------------------------- > > (Updated May 25, 2016, 9:42 p.m.) > > > Review request for geode, Ken Howe and Sai Boorlagadda. > > > Bugs: GEODE-1456 > https://issues.apache.org/jira/browse/GEODE-1456 > > > Repository: geode > > > Description > ------- > > fix race in GemFireCacheImplTest > > The race was caused by the GemFireCacheImpl constructor creating a real > TypeRegistry. > The TypeRegistry create a region which scheduled an async create region event > with the > event pool. So when the test set "initialCount" this create region event > might not have > completed so the initialCount would be zero. The test then schedule > MAX_THREADS tasks > and waits until that many complete. But the create region event could also > complete causing > getCompletedTaskCount() to keep return a value 1 more than the test expected. > > The test now mocks the TypeRegistry so the only events scheduled with the > pool come from > the unit test. > > > Diffs > ----- > > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java > 3ff162e692c5e9e14e1d6e2dd16fa8d5e3e02bef > > geode-core/src/test/java/com/gemstone/gemfire/internal/cache/GemFireCacheImplTest.java > 71b7fc6aecf77244a226b4853a1f5992b4de35af > > Diff: https://reviews.apache.org/r/47855/diff/ > > > Testing > ------- > > precheckin > > > Thanks, > > Darrel Schneider > >
