-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update provider to null #1452
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
/* | ||
* 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 com.alipay.sofa.rpc.client; | ||
|
||
import com.alipay.sofa.rpc.bootstrap.ConsumerBootstrap; | ||
import com.alipay.sofa.rpc.config.ConsumerConfig; | ||
import com.alipay.sofa.rpc.core.exception.SofaRpcException; | ||
import com.alipay.sofa.rpc.core.request.SofaRequest; | ||
import com.alipay.sofa.rpc.core.response.SofaResponse; | ||
import com.alipay.sofa.rpc.transport.ClientTransport; | ||
import org.junit.Assert; | ||
import org.junit.Test; | ||
|
||
import java.util.Arrays; | ||
import java.util.Collection; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Set; | ||
import java.util.concurrent.ConcurrentMap; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
|
||
/** | ||
* | ||
* @author junyuan | ||
* @version ClusterProviderUpdateTest.java, v 0.1 2024-10-11 11:04 junyuan Exp $ | ||
*/ | ||
public class ClusterProviderUpdateTest { | ||
private static final AbstractCluster cluster; | ||
|
||
static { | ||
ConsumerBootstrap bootstrap = new ConsumerBootstrap(new ConsumerConfig()) { | ||
@Override | ||
public Object refer() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public void unRefer() { | ||
|
||
} | ||
|
||
@Override | ||
public Object getProxyIns() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public Cluster getCluster() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public List<ProviderGroup> subscribe() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public boolean isSubscribed() { | ||
return false; | ||
} | ||
}; | ||
|
||
cluster = new AbstractCluster(bootstrap) { | ||
@Override | ||
protected SofaResponse doInvoke(SofaRequest msg) throws SofaRpcException { | ||
return null; | ||
} | ||
}; | ||
|
||
cluster.addressHolder = new SingleGroupAddressHolder(null); | ||
cluster.connectionHolder = new TestUseConnectionHolder(null); | ||
} | ||
|
||
@Test | ||
public void testUpdateProvider() { | ||
String groupName = "testUpdateProvider-Group"; | ||
List<ProviderInfo> providerList = Arrays.asList( | ||
ProviderHelper.toProviderInfo("127.0.0.1:12200"), | ||
ProviderHelper.toProviderInfo("127.0.0.1:12201"), | ||
ProviderHelper.toProviderInfo("127.0.0.1:12202")); | ||
ProviderGroup g = new ProviderGroup(groupName, providerList); | ||
cluster.updateProviders(g); | ||
|
||
Assert.assertEquals(cluster.currentProviderList().size(), providerList.size()); | ||
|
||
cluster.updateProviders(new ProviderGroup(groupName, null)); | ||
|
||
Assert.assertTrue(cluster.getAddressHolder().getProviderGroup(groupName).isEmpty()); | ||
Assert.assertEquals( 1, ((TestUseConnectionHolder)cluster.connectionHolder).calledCloseAllClientTransports.get()); | ||
} | ||
Comment on lines
+88
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance test coverage and assertions in testUpdateProvider method. The current test covers basic scenarios, but consider the following improvements:
Example of enhanced assertions: @Test
public void testUpdateProvider() {
String groupName = "testUpdateProvider-Group";
List<ProviderInfo> providerList = Arrays.asList(
ProviderHelper.toProviderInfo("127.0.0.1:12200"),
ProviderHelper.toProviderInfo("127.0.0.1:12201"),
ProviderHelper.toProviderInfo("127.0.0.1:12202"));
ProviderGroup g = new ProviderGroup(groupName, providerList);
cluster.updateProviders(g);
List<ProviderInfo> currentProviders = cluster.currentProviderList();
Assert.assertEquals("Provider list size should match", providerList.size(), currentProviders.size());
Assert.assertTrue("All providers should be present", currentProviders.containsAll(providerList));
// Test with empty list
cluster.updateProviders(new ProviderGroup(groupName, Collections.emptyList()));
Assert.assertTrue("Provider group should be empty", cluster.getAddressHolder().getProviderGroup(groupName).isEmpty());
// Test with null list
cluster.updateProviders(new ProviderGroup(groupName, null));
Assert.assertTrue("Provider group should be empty", cluster.getAddressHolder().getProviderGroup(groupName).isEmpty());
Assert.assertEquals("closeAllClientTransports should be called twice", 2, ((TestUseConnectionHolder)cluster.connectionHolder).calledCloseAllClientTransports.get());
} These enhancements will provide more comprehensive coverage and stronger assertions. |
||
|
||
|
||
private static class TestUseConnectionHolder extends ConnectionHolder { | ||
Set<ProviderInfo> connections = new HashSet<>(); | ||
|
||
AtomicInteger calledCloseAllClientTransports = new AtomicInteger(); | ||
|
||
/** | ||
* 构造函数 | ||
* | ||
* @param consumerBootstrap 服务消费者配置 | ||
*/ | ||
protected TestUseConnectionHolder(ConsumerBootstrap consumerBootstrap) { | ||
super(consumerBootstrap); | ||
} | ||
|
||
@Override | ||
public void closeAllClientTransports(DestroyHook destroyHook) { | ||
calledCloseAllClientTransports.getAndIncrement(); | ||
} | ||
|
||
@Override | ||
public ConcurrentMap<ProviderInfo, ClientTransport> getAvailableConnections() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public List<ProviderInfo> getAvailableProviders() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public ClientTransport getAvailableClientTransport(ProviderInfo providerInfo) { | ||
return null; | ||
} | ||
|
||
@Override | ||
public boolean isAvailableEmpty() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public Collection<ProviderInfo> currentProviderList() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public void setUnavailable(ProviderInfo providerInfo, ClientTransport transport) { | ||
|
||
} | ||
|
||
@Override | ||
public void addProvider(ProviderGroup providerGroup) { | ||
throw new UnsupportedOperationException("not implemented"); | ||
} | ||
|
||
@Override | ||
public void removeProvider(ProviderGroup providerGroup) { | ||
throw new UnsupportedOperationException("not implemented"); | ||
|
||
} | ||
|
||
@Override | ||
public void updateProviders(ProviderGroup providerGroup) { | ||
for (ProviderInfo i : providerGroup.getProviderInfos()) { | ||
connections.add(i); | ||
} | ||
} | ||
|
||
@Override | ||
public void updateAllProviders(List<ProviderGroup> providerGroups) { | ||
|
||
} | ||
|
||
@Override | ||
public void destroy() { | ||
|
||
} | ||
|
||
@Override | ||
public void destroy(DestroyHook hook) { | ||
|
||
} | ||
|
||
@Override | ||
public void init() { | ||
|
||
} | ||
} | ||
Comment on lines
+107
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance TestUseConnectionHolder for more comprehensive testing. The current implementation of TestUseConnectionHolder is minimal and might not provide enough flexibility for future tests. Consider the following improvements:
Example improvements: private static class TestUseConnectionHolder extends ConnectionHolder {
private final Map<ProviderInfo, ClientTransport> connections = new ConcurrentHashMap<>();
final AtomicInteger calledCloseAllClientTransports = new AtomicInteger();
final AtomicInteger calledAddProvider = new AtomicInteger();
final AtomicInteger calledRemoveProvider = new AtomicInteger();
// ... (existing constructor)
@Override
public ConcurrentMap<ProviderInfo, ClientTransport> getAvailableConnections() {
return new ConcurrentHashMap<>(connections);
}
@Override
public List<ProviderInfo> getAvailableProviders() {
return new ArrayList<>(connections.keySet());
}
@Override
public ClientTransport getAvailableClientTransport(ProviderInfo providerInfo) {
return connections.get(providerInfo);
}
@Override
public boolean isAvailableEmpty() {
return connections.isEmpty();
}
@Override
public Collection<ProviderInfo> currentProviderList() {
return new ArrayList<>(connections.keySet());
}
@Override
public void addProvider(ProviderGroup providerGroup) {
calledAddProvider.incrementAndGet();
updateProviders(providerGroup);
}
@Override
public void removeProvider(ProviderGroup providerGroup) {
calledRemoveProvider.incrementAndGet();
for (ProviderInfo info : providerGroup.getProviderInfos()) {
connections.remove(info);
}
}
@Override
public void updateProviders(ProviderGroup providerGroup) {
for (ProviderInfo info : providerGroup.getProviderInfos()) {
connections.put(info, null); // or mock ClientTransport if needed
}
}
@Override
public void updateAllProviders(List<ProviderGroup> providerGroups) {
connections.clear();
for (ProviderGroup group : providerGroups) {
updateProviders(group);
}
}
// ... (other methods remain the same)
} These enhancements will make the TestUseConnectionHolder more versatile for current and future tests. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using @BeforeClass instead of static initialization.
While the current setup allows for isolated testing of the cluster's behavior, using static initialization for test setup can lead to issues with test isolation and make it harder to reset the state between tests. Consider refactoring this to use JUnit's
@BeforeClass
annotation for setup, which would provide better control over the test lifecycle.Example refactoring:
This approach would maintain the same functionality while improving test isolation and maintainability.