-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
test : fix mockserverTest fail cause using same port with seata-server #6325
Conversation
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingServer.java
Outdated
Show resolved
Hide resolved
Please register this PR in changes |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #6325 +/- ##
============================================
+ Coverage 30.10% 30.34% +0.24%
- Complexity 5441 5508 +67
============================================
Files 930 936 +6
Lines 60222 60249 +27
Branches 7121 7088 -33
============================================
+ Hits 18129 18284 +155
+ Misses 40204 40041 -163
- Partials 1889 1924 +35 |
test/src/test/java/org/apache/seata/core/rpc/netty/mockserver/MockServerTest.java
Show resolved
Hide resolved
test/src/test/java/org/apache/seata/core/rpc/netty/mockserver/MockServerTest.java
Outdated
Show resolved
Hide resolved
test-mock-server/src/main/java/org/apache/seata/mockserver/MockServer.java
Outdated
Show resolved
Hide resolved
test-mock-server/src/main/java/org/apache/seata/mockserver/MockServer.java
Outdated
Show resolved
Hide resolved
putConfig(dataId, null); | ||
} | ||
|
||
public static void putConfig(String dataId, String content) { |
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.
You want to understand what the purpose of mock-server is? Connecting to the Seata-Server as if using a normal SDK eliminates the cumbersome Seata-Server setup, which is useful for testing client integration. If the user gets this mock-server, need to write such complex logic to test it, what is the point of it?
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.
I understand the point of mock-server, this class is only used for tests where you need to dynamically change the configuration in the unit test. It is only involved in the mock-server unit tests because it shares a key with TC-server.
Normal users don't need to use this test class to use mock-server, if they want to change the port they just need to use mock-server/resource/application.yml
This complexity arises because the unit tests for both mock-server and seata-server are in one package, or maybe we can create another test subproject for mock-server?
我理解mock-server的意义,这个类仅用于测试时需要动态改变单元测试里的配置。在mock-server的单元测试里因为和TC-server共用了一个key才会涉及它。
正常的用户在使用mock-server里是不需要用到这个test类的,他们如果想改变端口只需要使用mock-server/resource/application.yml
这个复杂性的出现是因为mock-server和seata-server的单元测试都在一个包里,又或者我们可以为mock-server再建一个test子项目?
test/src/test/java/org/apache/seata/common/ConfigurationTestHelper.java
Outdated
Show resolved
Hide resolved
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.
LGTM
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.
OK. However, I hope that the xid port and listen port can be set uniformly in the future.
Ⅰ. Describe what this PR did
mockserverTest fail cause using same port with seata-server
Ⅱ. Does this pull request fix one issue?
fixes #6320
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews