kirklund commented on a change in pull request #5978:
URL: https://github.com/apache/geode/pull/5978#discussion_r573360097
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
##########
@@ -1713,10 +1713,11 @@ private void setNewValueInRegion(final InternalRegion
owner, final RegionEntry r
// This is a horrible hack, but we need to get the size of the object
// When we store an entry. This code is only used when we do a put
// in the primary.
- if (v instanceof org.apache.geode.Delta &&
getRegion().isUsedForPartitionedRegionBucket()) {
+ if (v instanceof Delta && getRegion().isUsedForPartitionedRegionBucket()) {
int vSize;
Object ov = basicGetOldValue();
- if (ov instanceof CachedDeserializable &&
!GemFireCacheImpl.DELTAS_RECALCULATE_SIZE) {
+ if (ov instanceof CachedDeserializable &&
!(GemFireCacheImpl.DELTAS_RECALCULATE_SIZE
Review comment:
If you're looking for ideas on how to get started, one basic technique
from Working Effectively with Legacy Code is to extract your conditional as a
method (static or non-static) and provide unit tests for that new method:
```
@VisibleForTesting
static boolean shouldRecalculateSize(Delta deltaValue) {
return GemFireCacheImpl.DELTAS_RECALCULATE_SIZE ||
deltaValue.getForceRecalculateSize();
}
```
And use it inline where you need to change the behavior (note this could use
a LOT more cleanup but this is a start so you can start with unit test coverage:
```
if (ov instanceof CachedDeserializable &&
!shouldRecalculateSize((Delta)v)) {
```
And now some unit test coverage:
```
@Test
public void
shouldRecalculateSize_returnsTrue_ifDELTAS_RECALCULATE_SIZEisTrue() {
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE = true;
Delta deltaValue = mock(Delta.class);
when(deltaValue.getForceRecalculateSize())
.thenReturn(false);
boolean value = EntryEventImpl.shouldRecalculateSize(deltaValue);
assertThat(value).isTrue();
}
@Test
public void
shouldRecalculateSize_returnsFalse_ifDELTAS_RECALCULATE_SIZEisFalse() {
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE = false;
Delta deltaValue = mock(Delta.class);
when(deltaValue.getForceRecalculateSize())
.thenReturn(false);
boolean value = EntryEventImpl.shouldRecalculateSize(deltaValue);
assertThat(value).isFalse();
}
@Test
public void
shouldRecalculateSize_returnsTrue_ifGetForceRecalculateSizeIsTrue() {
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE = false;
Delta deltaValue = mock(Delta.class);
when(deltaValue.getForceRecalculateSize())
.thenReturn(true);
boolean value = EntryEventImpl.shouldRecalculateSize(deltaValue);
assertThat(value).isTrue();
}
@Test
public void
shouldRecalculateSize_returnsFalse_ifGetForceRecalculateSizeIsFalse() {
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE = false;
Delta deltaValue = mock(Delta.class);
when(deltaValue.getForceRecalculateSize())
.thenReturn(false);
boolean value = EntryEventImpl.shouldRecalculateSize(deltaValue);
assertThat(value).isFalse();
}
@Test
public void
shouldRecalculateSize_returnsTrue_ifGetForceRecalculateSizeIsFalse_andDELTAS_RECALCULATE_SIZEisTrue()
{
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE = true;
Delta deltaValue = mock(Delta.class);
when(deltaValue.getForceRecalculateSize())
.thenReturn(false);
boolean value = EntryEventImpl.shouldRecalculateSize(deltaValue);
assertThat(value).isTrue();
}
@After
public void tearDown() {
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE = false;
}
```
This way you can add test coverage, improve code readability, and make the
types a bit more clear. You can also extract a non-static method or do further
refactoring but you'll end up needing to do a lot more setUp in the unit tests.
These new tests should also make it obvious that
GemFireCacheImpl.DELTAS_RECALCULATE_SIZE should probably be moved and changed
and tested. But that's another PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
