kirklund commented on a change in pull request #7108: URL: https://github.com/apache/geode/pull/7108#discussion_r752688419
########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMember.java ########## @@ -0,0 +1,27 @@ +/* + * 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.management.internal.cli.commands; + +import java.util.ArrayList; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.management.internal.SystemManagementService; + +interface StopGatewaySenderOnMember { + ArrayList<String> executeStopGatewaySenderOnMember(String id, Cache cache, Review comment: Always declare with Interfaces instead of implementations: ``` List<String> executeStopGatewaySenderOnMember(...); ``` ########## File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandTest.java ########## @@ -0,0 +1,91 @@ +/* + * 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.management.internal.cli.commands; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; + +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.management.internal.cli.result.model.ResultModel; +import org.apache.geode.management.internal.functions.CliFunctionResult; +import org.apache.geode.test.junit.rules.GfshParserRule; + +public class StopGatewaySenderCommandTest { + @ClassRule + public static GfshParserRule gfsh = new GfshParserRule(); + + private StopGatewaySenderCommand command; + + @Before + public void before() { + command = spy(StopGatewaySenderCommand.class); + List<CliFunctionResult> functionResults = new ArrayList<>(); + doReturn(functionResults).when(command).executeAndGetFunctionResult(any(), any(), any()); Review comment: Anytime you override an instance method on the component being tested, you're either not testing some part of the component or it suggests that the method represents a responsibility that should not be on this component. You can avoid using `spy` and overriding the instance method by using a field of type `java.util.function.Function`, or a custom interface, that is injected into the constructor(s). 1. Add cleanResults field of type `java.util.function.Function` or custom inner-interface ``` private final java.util.function.Function<List<?>, List<CliFunctionResult>> cleanResults; ``` 2. Add constructors to `GfshCommand` ``` public GfshCommand() { this(CliFunctionResult::cleanResults); } protected GfshCommand(java.util.function.Function<List<?>, List<CliFunctionResult>> cleanResults) { this.cleanResults = cleanResults; } ``` 3. Add constructors to `StopGatewaySenderCommand` ``` @SuppressWarnings("unused") // invoked by spring shell public StopGatewaySenderCommand() { this(CliFunctionResult::cleanResults); } StopGatewaySenderCommand(Function<List<?>, List<CliFunctionResult>> cleanResults) { super(cleanResults); } ``` 4. Change `GfshCommand.executeAndGetFunctionResult` to use the field `cleanResults` ``` public List<CliFunctionResult> executeAndGetFunctionResult(Function<?> function, Object args, Set<DistributedMember> targetMembers) { ResultCollector<?, ?> rc = executeFunction(function, args, targetMembers); return cleanResults.apply((List<?>) rc.getResult()); } ``` 5. Pass in simple mock when constructing `StopGatewaySenderCommand` in test ``` Function<List<?>, List<CliFunctionResult>> cleanResults = mock(Function.class); command = new StopGatewaySenderCommand(cleanResults); ``` If you need to observe `cleanResults` to validate interaction with that dependency, you simply move it from a local variable to a field: ``` private Function<List<?>, List<CliFunctionResult>> cleanResults; // ... cleanResults = mock(Function.class); command = new StopGatewaySenderCommand(cleanResults); ``` Then you can perform any Mockito validation calls against `cleanResults` that you need to. ########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegate.java ########## @@ -0,0 +1,26 @@ +/* + * 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.management.internal.cli.commands; + +import java.util.Set; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.management.internal.cli.result.model.ResultModel; + +interface StopGatewaySenderCommandDelegate { Review comment: Remember to add `@FunctionalInterface` to any interface that will always define just one abstract method. ########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImpl.java ########## @@ -0,0 +1,64 @@ +/* + * 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.management.internal.cli.commands; + +import java.util.ArrayList; + +import javax.management.ObjectName; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.management.GatewaySenderMXBean; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.management.internal.i18n.CliStrings; + +class StopGatewaySenderOnMemberWithBeanImpl implements StopGatewaySenderOnMember { + + public ArrayList<String> executeStopGatewaySenderOnMember(String id, Cache cache, Review comment: Return `List<String>` instead of `ArrayList<String>` ########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImpl.java ########## @@ -0,0 +1,111 @@ +/* + * 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.management.internal.cli.commands; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; + +import org.apache.geode.annotations.VisibleForTesting; +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.logging.internal.executors.LoggingExecutors; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.management.internal.cli.result.model.ResultModel; +import org.apache.geode.management.internal.cli.result.model.TabularResultModel; +import org.apache.geode.management.internal.i18n.CliStrings; + +class StopGatewaySenderCommandDelegateParallelImpl + implements StopGatewaySenderCommandDelegate { + + private final ExecutorService executorService; + private final SystemManagementService managementService; + private final StopGatewaySenderOnMemberFactory stopperOnMemberFactory; + + @VisibleForTesting + public StopGatewaySenderCommandDelegateParallelImpl(ExecutorService executorService, + SystemManagementService managementService, + StopGatewaySenderOnMemberFactory stopperOnMemberFactory) { + this.executorService = executorService; + this.managementService = managementService; + this.stopperOnMemberFactory = stopperOnMemberFactory; + } + + public StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) { + executorService = LoggingExecutors.newCachedThreadPool("Stop Sender Command Thread ", true);; + stopperOnMemberFactory = new StopGatewaySenderOnMemberFactoryWithBeanImpl(); + this.managementService = managementService; + } + + public ResultModel executeStopGatewaySender(String id, Cache cache, + Set<DistributedMember> dsMembers) { + List<Callable<List<String>>> callables = new ArrayList<>(); + for (final DistributedMember member : dsMembers) { + callables.add(() -> stopperOnMemberFactory.create().executeStopGatewaySenderOnMember(id, + cache, managementService, member)); + } + + List<Future<List<String>>> futures; + try { + futures = executorService.invokeAll(callables); + } catch (InterruptedException ite) { + Thread.currentThread().interrupt(); + return ResultModel.createError( + CliStrings.format(CliStrings.GATEWAY_SENDER_STOP_0_COULD_NOT_BE_INVOKED_DUE_TO_1, id, + ite.getMessage())); + } finally { + executorService.shutdown(); + } + + return buildResultModelFromMembersResponses(id, dsMembers, futures); + } + + private ResultModel buildResultModelFromMembersResponses(String id, + Set<DistributedMember> dsMembers, List<Future<List<String>>> futures) { + ResultModel resultModel = new ResultModel(); + TabularResultModel resultData = resultModel.addTable(CliStrings.STOP_GATEWAYSENDER); + Iterator<DistributedMember> memberIterator = dsMembers.iterator(); + for (Future<List<String>> future : futures) { + DistributedMember member = memberIterator.next(); Review comment: Iterator doesn't guarantee order unless the declared collection type guarantees ordering of `iterator()`. It's also not clear what the relation ship is between dsMembers or futures in this method. I can't tell if the code is correct. I think you should make changes here that don't depend on ordering or some implied relationship between the two collections. ########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMember.java ########## @@ -0,0 +1,27 @@ +/* + * 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.management.internal.cli.commands; + +import java.util.ArrayList; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.management.internal.SystemManagementService; + +interface StopGatewaySenderOnMember { Review comment: `@FunctionalInterface` ########## File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.management.internal.cli.commands; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; + +import junitparams.Parameters; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.distributed.DistributedSystem; +import org.apache.geode.management.GatewaySenderMXBean; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public class StopGatewaySenderOnMemberWithBeanImplTest { + String senderId = "sender1"; + String memberId = "member1"; + String remoteMemberId = "remoteMember1"; + Cache cache; + SystemManagementService managementService; + DistributedMember distributedMember; + DistributedMember remoteDistributedMember; + DistributedSystem distributedSystem; + GatewaySenderMXBean bean; + + @Before + public void setUp() { + cache = mock(Cache.class); + bean = mock(GatewaySenderMXBean.class); + managementService = mock(SystemManagementService.class); + distributedMember = mock(DistributedMember.class); + remoteDistributedMember = mock(DistributedMember.class); + when(distributedMember.getId()).thenReturn(memberId); + when(remoteDistributedMember.getId()).thenReturn(remoteMemberId); + distributedSystem = mock(DistributedSystem.class); + doReturn(distributedSystem).when(cache).getDistributedSystem(); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, false); + doReturn(false).when(bean).isRunning(); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, Review comment: `List<String>` ########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImpl.java ########## @@ -0,0 +1,111 @@ +/* + * 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.management.internal.cli.commands; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; + +import org.apache.geode.annotations.VisibleForTesting; +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.logging.internal.executors.LoggingExecutors; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.management.internal.cli.result.model.ResultModel; +import org.apache.geode.management.internal.cli.result.model.TabularResultModel; +import org.apache.geode.management.internal.i18n.CliStrings; + +class StopGatewaySenderCommandDelegateParallelImpl + implements StopGatewaySenderCommandDelegate { + + private final ExecutorService executorService; + private final SystemManagementService managementService; + private final StopGatewaySenderOnMemberFactory stopperOnMemberFactory; + + @VisibleForTesting + public StopGatewaySenderCommandDelegateParallelImpl(ExecutorService executorService, + SystemManagementService managementService, + StopGatewaySenderOnMemberFactory stopperOnMemberFactory) { + this.executorService = executorService; + this.managementService = managementService; + this.stopperOnMemberFactory = stopperOnMemberFactory; + } + + public StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) { + executorService = LoggingExecutors.newCachedThreadPool("Stop Sender Command Thread ", true);; + stopperOnMemberFactory = new StopGatewaySenderOnMemberFactoryWithBeanImpl(); + this.managementService = managementService; + } Review comment: Remember to always chain constructors. Only one constructor should write to the fields. The other constructors call each other and finally the one true constructor. You generally want to order the constructors such that the widest scope is at the top and the one true constructor is the last one. Neither constructor needs to be public -- both can be package-private in this case. You can also delete `StopGatewaySenderOnMemberFactoryWithBeanImpl` and just inline `StopGatewaySenderOnMemberWithBeanImpl::new` for the factory argument. I recommend: ``` import static org.apache.geode.logging.internal.executors.LoggingExecutors.newCachedThreadPool; StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService) { this(managementService, newCachedThreadPool("Stop Sender Command Thread ", true), StopGatewaySenderOnMemberWithBeanImpl::new); } StopGatewaySenderCommandDelegateParallelImpl(SystemManagementService managementService, ExecutorService executorService, StopGatewaySenderOnMemberFactory stopperOnMemberFactory) { this.managementService = managementService; this.executorService = executorService; this.stopperOnMemberFactory = stopperOnMemberFactory; } ``` ########## File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.management.internal.cli.commands; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; + +import junitparams.Parameters; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.distributed.DistributedSystem; +import org.apache.geode.management.GatewaySenderMXBean; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public class StopGatewaySenderOnMemberWithBeanImplTest { + String senderId = "sender1"; + String memberId = "member1"; + String remoteMemberId = "remoteMember1"; + Cache cache; + SystemManagementService managementService; + DistributedMember distributedMember; + DistributedMember remoteDistributedMember; + DistributedSystem distributedSystem; + GatewaySenderMXBean bean; + + @Before + public void setUp() { + cache = mock(Cache.class); + bean = mock(GatewaySenderMXBean.class); + managementService = mock(SystemManagementService.class); + distributedMember = mock(DistributedMember.class); + remoteDistributedMember = mock(DistributedMember.class); + when(distributedMember.getId()).thenReturn(memberId); + when(remoteDistributedMember.getId()).thenReturn(remoteMemberId); + distributedSystem = mock(DistributedSystem.class); + doReturn(distributedSystem).when(cache).getDistributedSystem(); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, false); + doReturn(false).when(bean).isRunning(); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, + managementService, distributedMember); + assertThat(result).containsExactly(memberId, "Error", + "GatewaySender sender1 is not running on member " + memberId + "."); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, true); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, + managementService, distributedMember); + assertThat(result).containsExactly(memberId, "Error", + "GatewaySender sender1 is not available on member " + memberId); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, false); + doReturn(true).when(bean).isRunning(); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, + managementService, distributedMember); + assertThat(result).containsExactly(memberId, "OK", + "GatewaySender sender1 is stopped on member " + memberId); + } + + private void setUpMocks(boolean isLocalMember, boolean beanMustBeNull) { Review comment: How about more descriptive names like: ``` private GatewaySenderMXBean gatewaySenderMXBean(boolean isLocalMember, boolean mustBeNull) { GatewaySenderMXBean gatewaySenderMXBean = mustBeNull ? null : mock(GatewaySenderMXBean.class); // ... return gatewaySenderMXBean; } ``` You can then delete the field `private GatewaySenderMXBean bean;` and change the tests do something like: ``` @Test @Parameters({"true", "false"}) public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) { GatewaySenderMXBean gatewaySenderMXBean = gatewaySenderMXBean(isLocalMember, false); when(gatewaySenderMXBean.isRunning()).thenReturn(true); StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); List<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, managementService, distributedMember); assertThat(result) .containsExactly(memberId, "OK", "GatewaySender sender1 is stopped on member " + memberId); } ``` ########## File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderCommandDelegateParallelImplTest.java ########## @@ -0,0 +1,126 @@ +/* + * 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.management.internal.cli.commands; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.management.internal.cli.result.model.ResultModel; +import org.apache.geode.management.internal.cli.result.model.TabularResultModel; +import org.apache.geode.management.internal.i18n.CliStrings; + +public class StopGatewaySenderCommandDelegateParallelImplTest { + private final String senderId = "sender1"; + private Cache cache; + Set<DistributedMember> members; + ExecutorService executorService; + SystemManagementService managementService; + StopGatewaySenderOnMemberFactory stopperOnMemberFactory; Review comment: These should be `private` unless you subclass the test itself in which case `protected` might be appropriate. ########## File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.management.internal.cli.commands; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; + +import junitparams.Parameters; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.distributed.DistributedSystem; +import org.apache.geode.management.GatewaySenderMXBean; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public class StopGatewaySenderOnMemberWithBeanImplTest { + String senderId = "sender1"; + String memberId = "member1"; + String remoteMemberId = "remoteMember1"; + Cache cache; + SystemManagementService managementService; + DistributedMember distributedMember; + DistributedMember remoteDistributedMember; + DistributedSystem distributedSystem; + GatewaySenderMXBean bean; + + @Before + public void setUp() { + cache = mock(Cache.class); + bean = mock(GatewaySenderMXBean.class); + managementService = mock(SystemManagementService.class); + distributedMember = mock(DistributedMember.class); + remoteDistributedMember = mock(DistributedMember.class); + when(distributedMember.getId()).thenReturn(memberId); + when(remoteDistributedMember.getId()).thenReturn(remoteMemberId); + distributedSystem = mock(DistributedSystem.class); + doReturn(distributedSystem).when(cache).getDistributedSystem(); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, false); + doReturn(false).when(bean).isRunning(); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, + managementService, distributedMember); + assertThat(result).containsExactly(memberId, "Error", + "GatewaySender sender1 is not running on member " + memberId + "."); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, true); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, + managementService, distributedMember); + assertThat(result).containsExactly(memberId, "Error", + "GatewaySender sender1 is not available on member " + memberId); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberRunningReturnsOk(boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, false); + doReturn(true).when(bean).isRunning(); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, Review comment: `List<String>` ########## File path: geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberWithBeanImplTest.java ########## @@ -0,0 +1,113 @@ +/* + * 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.management.internal.cli.commands; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; + +import junitparams.Parameters; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.apache.geode.cache.Cache; +import org.apache.geode.distributed.DistributedMember; +import org.apache.geode.distributed.DistributedSystem; +import org.apache.geode.management.GatewaySenderMXBean; +import org.apache.geode.management.internal.SystemManagementService; +import org.apache.geode.test.junit.runners.GeodeParamsRunner; + +@RunWith(GeodeParamsRunner.class) +public class StopGatewaySenderOnMemberWithBeanImplTest { + String senderId = "sender1"; + String memberId = "member1"; + String remoteMemberId = "remoteMember1"; + Cache cache; + SystemManagementService managementService; + DistributedMember distributedMember; + DistributedMember remoteDistributedMember; + DistributedSystem distributedSystem; + GatewaySenderMXBean bean; + + @Before + public void setUp() { + cache = mock(Cache.class); + bean = mock(GatewaySenderMXBean.class); + managementService = mock(SystemManagementService.class); + distributedMember = mock(DistributedMember.class); + remoteDistributedMember = mock(DistributedMember.class); + when(distributedMember.getId()).thenReturn(memberId); + when(remoteDistributedMember.getId()).thenReturn(remoteMemberId); + distributedSystem = mock(DistributedSystem.class); + doReturn(distributedSystem).when(cache).getDistributedSystem(); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotRunningReturnsNotRunningError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, false); + doReturn(false).when(bean).isRunning(); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, + managementService, distributedMember); + assertThat(result).containsExactly(memberId, "Error", + "GatewaySender sender1 is not running on member " + memberId + "."); + } + + @Test + @Parameters({"true", "false"}) + public void executeStopGatewaySenderOnMemberNotAvailableReturnsNotAvailableError( + boolean isLocalMember) { + StopGatewaySenderOnMember stopperWithBean = new StopGatewaySenderOnMemberWithBeanImpl(); + setUpMocks(isLocalMember, true); + ArrayList<String> result = stopperWithBean.executeStopGatewaySenderOnMember(senderId, cache, Review comment: `List<String>` ########## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StopGatewaySenderOnMemberFactory.java ########## @@ -0,0 +1,20 @@ +/* + * 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.management.internal.cli.commands; + +interface StopGatewaySenderOnMemberFactory { Review comment: `@FunctionalInterface` Since this interface is so simple (no arguments to `create`), you could simply use `java.util.function.Supplier` instead. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
