Skip to content
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

Change main method under dubbo-remoting-api to standard test #5026

Merged
merged 6 commits into from
Sep 10, 2019

Conversation

xiaomoran
Copy link
Contributor

Because of the commits linked to the wrong user, I resubmitted a pr and continued #4957 @htynkn

@xiaomoran xiaomoran closed this Sep 7, 2019
@xiaomoran xiaomoran reopened this Sep 7, 2019
@xiaomoran xiaomoran closed this Sep 7, 2019
Copy link
Member

@htynkn htynkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. expect that for loop. maybe remove it if it's not necessary for unit test

System.out.println("handle:" + msg + ";thread:" + Thread.currentThread().getName());
return new StringMessage("hello world");
dispatcher.addReplier(Data.class, (channel, msg) -> {
for (int i = 0; i < 10000; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is existing code. but I'm thinking what's purpose for this code? looks like this is not necessary for this unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this loop seems to be unnecessary, just to keep the existing code. I have changed it.

Copy link
Member

@htynkn htynkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. will merge it once CI pass

@codecov-io
Copy link

Codecov Report

Merging #5026 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5026      +/-   ##
============================================
- Coverage     63.99%   63.93%   -0.07%     
- Complexity      451      453       +2     
============================================
  Files           769      769              
  Lines         33176    33194      +18     
  Branches       5230     5236       +6     
============================================
- Hits          21231    21222       -9     
- Misses         9523     9551      +28     
+ Partials       2422     2421       -1
Impacted Files Coverage Δ Complexity Δ
.../remoting/transport/netty4/NettyServerHandler.java 61.9% <0%> (-9.53%) 0% <0%> (ø)
.../apache/dubbo/rpc/protocol/AsyncToSyncInvoker.java 79.16% <0%> (-8.34%) 0% <0%> (ø)
...ng/transport/dispatcher/all/AllChannelHandler.java 51.42% <0%> (-5.72%) 0% <0%> (ø)
...ng/exchange/support/header/HeartbeatTimerTask.java 73.68% <0%> (-5.27%) 0% <0%> (ø)
.../org/apache/dubbo/remoting/ExecutionException.java 15.78% <0%> (-5.27%) 0% <0%> (ø)
...bbo/registry/support/ProviderConsumerRegTable.java 80.48% <0%> (-4.88%) 0% <0%> (ø)
...onfig/spring/extension/SpringExtensionFactory.java 80.48% <0%> (-4.88%) 0% <0%> (ø)
...exchange/support/header/HeaderExchangeHandler.java 64.75% <0%> (-2.46%) 0% <0%> (ø)
...he/dubbo/registry/multicast/MulticastRegistry.java 67.87% <0%> (-1.81%) 0% <0%> (ø)
...va/org/apache/dubbo/rpc/support/ProtocolUtils.java 70.37% <0%> (-1.63%) 0% <0%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cb034c...4d92e58. Read the comment docs.

@htynkn htynkn merged commit 9275b9d into apache:master Sep 10, 2019
@htynkn
Copy link
Member

htynkn commented Sep 10, 2019

Thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants