[GitHub] [geode] kirklund commented on a change in pull request #5208: GEODE-8173: Add unit test (coverage) for PartitionedRegionClear class.

[GitBox <gi...@apache.org>]

kirklund commented on a change in pull request #5208:
URL: https://github.com/apache/geode/pull/5208#discussion_r436075685



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -10269,4 +10269,8 @@ boolean hasAnyClientsInterested() {
     return (getRegionAdvisor().hasPRServerWithInterest()
         || getRegionAdvisor().hasPRServerWithCQs());
   }
+
+  boolean isTransactionDistributed() {

Review comment:
       I hate the idea of adding methods like this to Regions. It has nothing to do with Region. Maybe Transaction or TransactionManager would be a better place for this method.

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionClearTest.java
##########
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.PartitionedRegionPartialClearException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedLockService;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.MembershipListener;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.partitioned.RegionAdvisor;
+import org.apache.geode.internal.cache.versions.RegionVersionVector;
+
+public class PartitionedRegionClearTest {
+
+
+  private PartitionedRegionClear partitionedRegionClear;
+  private DistributionManager distributionManager;
+  private PartitionedRegion partitionedRegion;
+
+  @Before
+  public void setUp() {
+
+    partitionedRegion = mock(PartitionedRegion.class);
+    distributionManager = mock(DistributionManager.class);
+
+    when(partitionedRegion.getDistributionManager()).thenReturn(distributionManager);
+    when(partitionedRegion.getName()).thenReturn("prRegion");
+
+    partitionedRegionClear = new PartitionedRegionClear(partitionedRegion);
+  }
+
+  private Set<BucketRegion> setupBucketRegions(
+      PartitionedRegionDataStore partitionedRegionDataStore,
+      BucketAdvisor bucketAdvisor) {
+    final int numBuckets = 2;
+    HashSet<BucketRegion> bucketRegions = new HashSet<>();

Review comment:
       Let's move away from the Jason Penney quirks. We should be declaring vars as the interface instead of the impl:
   ```
   Set<BucketRegion> bucketRegions = new HashSet<>();
   ```

##########
File path: geode-core/src/distributedTest/java/org/apache/geode/cache/ReplicateCacheListenerDistributedTest.java
##########
@@ -179,7 +180,13 @@ public void afterRegionDestroyIsInvokedInEveryMember() {
 
     region.destroyRegion();
 
-    assertThat(sharedCountersRule.getTotal(REGION_DESTROY)).isEqualTo(expectedRegionDestroys());
+    if (region instanceof PartitionedRegion) {

Review comment:
       You typically want to avoid if-else blocks like this, and separate it into two different tests. One for PR and one for Replicate.
   
   The test class is ReplicateCacheListenerDistributedTest. I can't find any PartitionedRegions being created in this test. Does this belong in PRCacheListenerDistributedTest instead?

##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionClearTest.java
##########
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.PartitionedRegionPartialClearException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedLockService;
+import org.apache.geode.distributed.internal.DMStats;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.MembershipListener;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.partitioned.RegionAdvisor;
+import org.apache.geode.internal.cache.versions.RegionVersionVector;
+
+public class PartitionedRegionClearTest {
+
+
+  private PartitionedRegionClear partitionedRegionClear;
+  private DistributionManager distributionManager;
+  private PartitionedRegion partitionedRegion;
+
+  @Before
+  public void setUp() {
+
+    partitionedRegion = mock(PartitionedRegion.class);
+    distributionManager = mock(DistributionManager.class);
+
+    when(partitionedRegion.getDistributionManager()).thenReturn(distributionManager);
+    when(partitionedRegion.getName()).thenReturn("prRegion");
+
+    partitionedRegionClear = new PartitionedRegionClear(partitionedRegion);
+  }
+
+  private Set<BucketRegion> setupBucketRegions(
+      PartitionedRegionDataStore partitionedRegionDataStore,
+      BucketAdvisor bucketAdvisor) {
+    final int numBuckets = 2;
+    HashSet<BucketRegion> bucketRegions = new HashSet<>();

Review comment:
       We should be declaring vars as the interface instead of the impl:
   ```
   Set<BucketRegion> bucketRegions = new HashSet<>();
   ```




----------------------------------------------------------------
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:
users@infra.apache.org