DonalEvans commented on a change in pull request #4818:
URL: https://github.com/apache/geode/pull/4818#discussion_r414861362
##########
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ClearCommandTest.java
##########
@@ -0,0 +1,93 @@
+package org.apache.geode.management.internal.cli.commands;
+
+import static
org.apache.geode.management.internal.i18n.CliStrings.CLEAR_REGION;
+import static
org.apache.geode.management.internal.i18n.CliStrings.CLEAR_REGION_REGION_NAME;
+import static
org.apache.geode.management.internal.cli.commands.ClearCommand.REGION_NOT_FOUND;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+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.when;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class ClearCommandTest {
Review comment:
This feels like more of an integration test as it's currently written,
since it's not directly testing the ClearCommand class but rather how that
class is used by gfsh. Instead of using `gfsh.executeAndAssertThat`, it might
better to call `ClearCommand.clear()` directly and assert on the returned
`ResultModel`.
It would also be good to verify that `callFunctionForRegion()` and
`clearfn.remove()` are called with the expected/correct arguments, rater than
just asserting the success/failure of the command, since otherwise the last two
test cases end up being indistinguishable in terms of what they're asserting
and the actual difference in behaviour between the two code paths isn't being
verified.
##########
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ClearCommandTest.java
##########
@@ -0,0 +1,93 @@
+package org.apache.geode.management.internal.cli.commands;
+
+import static
org.apache.geode.management.internal.i18n.CliStrings.CLEAR_REGION;
+import static
org.apache.geode.management.internal.i18n.CliStrings.CLEAR_REGION_REGION_NAME;
+import static
org.apache.geode.management.internal.cli.commands.ClearCommand.REGION_NOT_FOUND;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+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.when;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class ClearCommandTest {
+
+ @ClassRule
+ public static GfshParserRule gfsh = new GfshParserRule();
+
+ static final String regionName = "regionName";
+ static final String success = "SUCCESS";
+
+ InternalCache cache;
+ ClearCommand command;
+ Region region;
+ Set<DistributedMember> membersList;
+ DistributedMember member;
+ DataCommandResult dataResult;
+
+ @Before
+ public void setup() {
+ cache = mock(InternalCache.class);
+ command = spy(new ClearCommand());
+ region = mock(Region.class);
+ dataResult = mock(DataCommandResult.class);
+
+ membersList = new HashSet<DistributedMember>();
Review comment:
It's not necessary to specify the `DistributedMember` here, since it's
implicit in the declaration of the `membersList` set.
##########
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ClearCommandTest.java
##########
@@ -0,0 +1,93 @@
+package org.apache.geode.management.internal.cli.commands;
+
+import static
org.apache.geode.management.internal.i18n.CliStrings.CLEAR_REGION;
+import static
org.apache.geode.management.internal.i18n.CliStrings.CLEAR_REGION_REGION_NAME;
+import static
org.apache.geode.management.internal.cli.commands.ClearCommand.REGION_NOT_FOUND;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+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.when;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.Region;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.internal.cli.domain.DataCommandRequest;
+import org.apache.geode.management.internal.cli.domain.DataCommandResult;
+import org.apache.geode.management.internal.cli.functions.DataCommandFunction;
+import org.apache.geode.management.internal.cli.result.model.ResultModel;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+
+public class ClearCommandTest {
+
+ @ClassRule
+ public static GfshParserRule gfsh = new GfshParserRule();
+
+ static final String regionName = "regionName";
+ static final String success = "SUCCESS";
+
+ InternalCache cache;
+ ClearCommand command;
+ Region region;
Review comment:
To prevent compiler warnings, this should probably be `Region<Object,
Object>` and a `@SuppressWarnings("unchecked")` tag should be added to the
`setup()` method.
----------------------------------------------------------------
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:
us...@infra.apache.org
