Skip to content

add consul service discovery test#5699

Merged
chickenlj merged 2 commits intoapache:masterfrom
Moriadry-zz:feature/add-test
Feb 1, 2020
Merged

add consul service discovery test#5699
chickenlj merged 2 commits intoapache:masterfrom
Moriadry-zz:feature/add-test

Conversation

@Moriadry-zz
Copy link
Copy Markdown

@Moriadry-zz Moriadry-zz commented Jan 30, 2020

What is the purpose of the change

complete the unit test for consul service discovery module.

Brief changelog

XXXXX

Verifying this change

XXXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GITHUB_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
  • If this contribution is large, please follow the Software Donation Guide.

@chickenlj
Copy link
Copy Markdown
Contributor

chickenlj commented Jan 31, 2020

Have you considered making it into a integration test by using some frameworks like TestContainer that leverage Container?

@Moriadry-zz
Copy link
Copy Markdown
Author

Moriadry-zz commented Jan 31, 2020 •
edited
Loading

@chickenlj Can you show me any example so I can find whether I can dig in it.

I thought with embedded-consul imported, It should be an integration test somehow?

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 31, 2020

Codecov Report

Merging #5699 into master will increase coverage by 0.07%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5699      +/-   ##
============================================
+ Coverage     61.37%   61.45%   +0.07%     
- Complexity      425      492      +67     
============================================
  Files           923      926       +3     
  Lines         37572    37800     +228     
  Branches       5442     5453      +11     
============================================
+ Hits          23060    23229     +169     
- Misses        12032    12063      +31     
- Partials       2480     2508      +28
Impacted Files Coverage Δ Complexity Δ
.../dubbo/registry/consul/ConsulServiceDiscovery.java 65.06% <50%> (+65.06%) 21 <1> (+21) ⬆️
...rg/apache/dubbo/remoting/utils/PayloadDropper.java 46.15% <0%> (-30.77%) 0% <0%> (ø)
...ng/transport/dispatcher/all/AllChannelHandler.java 62.06% <0%> (-13.8%) 0% <0%> (ø)
...va/org/apache/dubbo/remoting/TimeoutException.java 22.22% <0%> (-11.12%) 0% <0%> (ø)
...e/dubbo/remoting/transport/netty/NettyChannel.java 52.27% <0%> (-7.96%) 19% <0%> (-2%)
.../remoting/transport/netty4/NettyClientHandler.java 59.32% <0%> (-6.78%) 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%> (ø)
.../apache/dubbo/remoting/transport/AbstractPeer.java 63.04% <0%> (-4.35%) 0% <0%> (ø)
...pache/dubbo/remoting/transport/AbstractServer.java 53.75% <0%> (-3.75%) 0% <0%> (ø)
... and 17 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 e234a89...e514e9f. Read the comment docs.

@chickenlj
Copy link
Copy Markdown
Contributor

chickenlj commented Feb 1, 2020

@chickenlj Can you show me any example so I can find whether I can dig in it.

I thought with embedded-consul imported, It should be an integration test somehow?

Yes, I think we can go with embedded Consul, I noticed it also helped us with the potential port conflicting problem.

You can reference here for how to use testcontainers doing Integration Test https://www.testcontainers.org/

@chickenlj chickenlj merged commit 8c1036e into apache:master Feb 1, 2020
@Moriadry-zz Moriadry-zz deleted the feature/add-test branch February 1, 2020 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Moriadry-zz @chickenlj @codecov-io @moriadry