You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "OrDTesters (GitHub)" <gi...@apache.org> on 2018/11/20 21:01:56 UTC
[GitHub] [incubator-dubbo] OrDTesters opened pull request #2807:
Fixing test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
## What is the purpose of the change
There are test-order dependencies in org.apache.dubbo.rpc.cluster.StickyTest. testStickyNoCheck fails when run after testStickyForceCheck. The tests depend on the same LoadBalance instance, which enters a different state after testStickyForceCheck runs, which then results in testStickNoCheck to fail when it runs.
The proposed fix introduces a method in ExtensionLoader to reset cached ExtensionLoader instances, for testing purposes only. StickyTest can then use this method to reset the ExtensionLoader for LoadBalance between test runs.
## Brief changelog
dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/StickyTest.java
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #2807: Fixing
test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I will have a check.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #2807: Fixing
test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I can reproduce this problem.
I am not sure if this modification is best. Maybe someone else can give a review.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] OrDTesters commented on pull request
#2807: Fixing test-order dependency in
org.apache.dubbo.rpc.cluster.StickyTest
Posted by "OrDTesters (GitHub)" <gi...@apache.org>.
When either testStickyForceCheck or testStickyNoCheck runs, they eventually end up using an instance of RoundRobinLoadBalance, which is obtained and shared between the two tests through the ExtensionLoader, accessed using the LoadBalance.class as key. In between the two runs, one of the shared RoundRobinLoadBalance instance's internal map fields changes. The change is such that if testStickyNoCheck runs after testStickyForceCheck, the assertion for testStickyNoCheck fails. The goal of resetting is to make sure the two tests do not end up using the same RoundRobinLoadBalance instance when they run.
I would be happy to discuss any other ways of approaching this problem.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 closed pull request #2807: Fixing
test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ pull request closed by beiwei30 ]
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] carryxyh commented on issue #2807: Fixing
test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I can't reproduce this problem on my mac.
I tried:
1. run testStickyForceCheck and then run testStickyNoCheck. UT passed.
2.run the whole test. UT passed.
3.move the test method testStickyForceCheck before testStickyNoCheck. UT passed.
Is my reproduce way is wrong?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] codecov-io commented on issue #2807:
Fixing test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "codecov-io (GitHub)" <gi...@apache.org>.
# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=h1) Report
> Merging [#2807](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/5ea6b33f2b270d83c37726750e1395f0972636c0?src=pr&el=desc) will **increase** coverage by `1.03%`.
> The diff coverage is `88.88%`.
[](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2807 +/- ##
=========================================
+ Coverage 62.46% 63.5% +1.03%
=========================================
Files 599 577 -22
Lines 27919 25939 -1980
Branches 4829 4543 -286
=========================================
- Hits 17440 16472 -968
+ Misses 8213 7302 -911
+ Partials 2266 2165 -101
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...apache/dubbo/common/extension/ExtensionLoader.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vZXh0ZW5zaW9uL0V4dGVuc2lvbkxvYWRlci5qYXZh) | `80.78% <88.88%> (+0.14%)` | :arrow_up: |
| [.../remoting/transport/netty4/NettyClientHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5Q2xpZW50SGFuZGxlci5qYXZh) | `75% <0%> (-11.12%)` | :arrow_down: |
| [...he/dubbo/remoting/transport/netty/NettyClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlbW90aW5nL3RyYW5zcG9ydC9uZXR0eS9OZXR0eUNsaWVudC5qYXZh) | `72.88% <0%> (-10.17%)` | :arrow_down: |
| [.../apache/dubbo/remoting/transport/AbstractPeer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RQZWVyLmphdmE=) | `58.69% <0%> (-8.7%)` | :arrow_down: |
| [...in/java/org/apache/dubbo/common/utils/JVMUtil.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9jb21tb24vdXRpbHMvSlZNVXRpbC5qYXZh) | `73.58% <0%> (-7.55%)` | :arrow_down: |
| [...ng/exchange/support/header/HeartbeatTimerTask.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9leGNoYW5nZS9zdXBwb3J0L2hlYWRlci9IZWFydGJlYXRUaW1lclRhc2suamF2YQ==) | `73.68% <0%> (-5.27%)` | :arrow_down: |
| [.../apache/dubbo/qos/protocol/QosProtocolWrapper.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcGx1Z2luL2R1YmJvLXFvcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcW9zL3Byb3RvY29sL1Fvc1Byb3RvY29sV3JhcHBlci5qYXZh) | `64.1% <0%> (-5.13%)` | :arrow_down: |
| [...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcnBjL2R1YmJvLXJwYy1kdWJiby9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZHViYm8vcnBjL3Byb3RvY29sL2R1YmJvL3RlbG5ldC9JbnZva2VUZWxuZXRIYW5kbGVyLmphdmE=) | `54.21% <0%> (-3.62%)` | :arrow_down: |
| [...pache/dubbo/remoting/transport/AbstractServer.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RTZXJ2ZXIuamF2YQ==) | `45.36% <0%> (-3.1%)` | :arrow_down: |
| [...pache/dubbo/remoting/transport/AbstractClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RDbGllbnQuamF2YQ==) | `65.94% <0%> (-2.17%)` | :arrow_down: |
| ... and [25 more](https://codecov.io/gh/apache/incubator-dubbo/pull/2807/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=footer). Last update [5ea6b33...2224f8d](https://codecov.io/gh/apache/incubator-dubbo/pull/2807?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #2807:
Fixing test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
I think I understand what happens now, agree with your change.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] OrDTesters commented on issue #2807:
Fixing test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "OrDTesters (GitHub)" <gi...@apache.org>.
The two tests have to be run in the same JVM. The order of the tests written in the file does not control the order that they are actually run by JUnit. To reproduce this issue, there is a way to force JUnit to run the tests in a specific order.
Add the annotation @org.junit.FixMethodOrder(org.junit.runners.MethodSorters.NAME_ASCENDING) to the test class. This will force JUnit to run the tests in the ascending alphabetical order of their names, so testStickyForceCheck will run before testStickyNoCheck.
Since without FixMethodOrder the order of the tests is not guaranteed, it is best to make sure the tests can pass when they are run in any order.
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on pull request #2807:
Fixing test-order dependency in org.apache.dubbo.rpc.cluster.StickyTest
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
I am not convinced either. Why we need to reset extension loader here?
[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2807 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org