Refactor and Test server core query package#2713
Refactor and Test server core query package#2713flycash wants to merge 14 commits intoapache:masterfrom
Conversation
|
#2654 is planned to be merged after UI updated. I prefer you could adopt that after merged. |
wu-sheng
added
TBD
|
okay |
|
#2654 merged. Please update this pull request. |
flycash
force-pushed
the
test-server-core-query
branch
3 times, most recently
from
5d8fc58 to
877d478
Compare
flycash
force-pushed
the
test-server-core-query
branch
2 times, most recently
from
a291286 to
8224590
Compare
...ver/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/DurationUtils.java
Outdated
Show resolved
Hide resolved
flycash
force-pushed
the
test-server-core-query
branch
from
8224590 to
bfe1bb9
Compare
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
FYI @IanCao @JaredTan95 This ps is a little hard to review, needs to test in local env.
|
Also, this title doesn't fit the PR very well. This PR includes a lot of refactors to codes. |
flycash
changed the title
|
Coverage increased (+1.8%) to 18.613% when pulling 8224590ce7966c173798129a4dc858c5494cc389 on flycash:test-server-core-query into 1530a66 on apache:master. |
|
Coverage increased (+1.8%) to 18.652% when pulling a2e874d8fc2bee2641e54d4b91efa74e1c24cf6f on flycash:test-server-core-query into e0827e3 on apache:master. |
wu-sheng
added
feature
There was a problem hiding this comment.
I think we need comments of which is the mock topology. Put them in the AbstractTopologyTest comments. And use logic variable name as static attribute/field to replace the numbers in assertTopology, mockClientCall , mockServerCall.
There was a problem hiding this comment.
Also mapping means client and server sides have agents, so the client -> peer could be translated to client -> server. So, I need to describe the topology clear. Otherwise, it is hard to say that, your test cases are right and scenarios coveraged.
There was a problem hiding this comment.
We need a tearDown to clear ValueColumnIds.INSTANCE, right?
There was a problem hiding this comment.
typo, expecteName -> expectedName
There was a problem hiding this comment.
The name should come from mock inventory, right? You put a name here without any comment, make reader very hard to understand. Suggest you put a comment,
Mock the entity name. This name should be replaced by mock inventory.
There was a problem hiding this comment.
Why put the DATE_STR in this format? It is not clear.
There was a problem hiding this comment.
Please follow the above review and make this assert clearer.
There was a problem hiding this comment.
assertNode is very hard to understand. Please use logic name to replace numbers.
|
I would add the comments but it would take some time due to my terrible English. |
|
Take your time. I could help you with English when you have a draft, and make the test codes more readable. |
flycash
force-pushed
the
test-server-core-query
branch
from
a2e874d to
7019816
Compare
3 participants
Add unit test for org.apache.skywalking.oap.server.core.query package
Why submit this pull request?
Bug fix
New feature provided
Improve performance
Related issues
Bug fix
Bug description.
How to fix?
New feature or improvement