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.

Sign up for GitHub

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

Jump to bottom

Add logic-endpoint and testcase for elasticjob-2.x plugin #5395

Merged
merged 17 commits into from
Aug 28, 2020
Merged

Add logic-endpoint and testcase for elasticjob-2.x plugin #5395

merged 17 commits into from
Aug 28, 2020

Conversation

hailin0
Copy link
Contributor

@hailin0 hailin0 commented Aug 26, 2020

Please answer these questions before submitting a pull request

  • 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

  • Describe the details and related test reports.

@hailin0 hailin0 changed the title Add logic-endpoint and testcase for elasticjob-2.x plugin 5f282b6 Add logic-endpoint and testcase for elasticjob-2.x plugin Aug 26, 2020
@codecov
Copy link

codecov bot commented Aug 26, 2020 •
edited

Codecov Report

Merging #5395 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5395      +/-   ##
============================================
+ Coverage     52.96%   53.00%   +0.03%     
- Complexity     3214     3218       +4     
============================================
  Files           831      832       +1     
  Lines         20977    20981       +4     
  Branches       2037     2037              
============================================
+ Hits          11111    11120       +9     
+ Misses         8957     8952       -5     
  Partials        909      909
Impacted Files Coverage Δ Complexity Δ
.../core/logging/core/coverts/ThrowableConverter.java 18.18% <0.00%> (-63.64%) 2.00% <0.00%> (-2.00%)
...ent/kafka/provider/handler/ProfileTaskHandler.java 26.08% <0.00%> (-47.83%) 4.00% <0.00%> (-1.00%)
...kywalking/apm/agent/core/jvm/JVMMetricsSender.java 82.35% <0.00%> (-5.89%) 11.00% <0.00%> (ø%)
...ing/apm/agent/core/logging/core/PatternLogger.java 56.75% <0.00%> (-2.71%) 18.00% <0.00%> (-2.00%)
...r/storage/plugin/influxdb/query/MetadataQuery.java 59.23% <0.00%> (-0.77%) 11.00% <0.00%> (-1.00%)
...alking/apm/agent/core/context/util/PeerFormat.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...er/sharing/server/SharingServerModuleProvider.java 44.92% <0.00%> (+1.44%) 8.00% <0.00%> (ø%)
...rary/client/elasticsearch/ElasticSearchClient.java 59.65% <0.00%> (+4.29%) 28.00% <0.00%> (+1.00%)
...ing/oap/server/library/server/grpc/GRPCServer.java 61.01% <0.00%> (+5.08%) 7.00% <0.00%> (+1.00%)
...r/cluster/plugin/standalone/StandaloneManager.java 100.00% <0.00%> (+22.22%) 4.00% <0.00%> (+1.00%)
... and 3 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 f89ed0c...56ffbf8. Read the comment docs.

@hailin0
Copy link
Contributor Author

hailin0 commented Aug 26, 2020

8AA7C57643D1D8123100DE06DC9DDC52
C1443EB762E3FCF5FEC7541C8DFE099C
3B86F827CD06ECB47C2D9458786FFC5F

@hailin0
Copy link
Contributor Author

hailin0 commented Aug 26, 2020 •
edited

Need to remove old unit tests？

@wu-sheng
Copy link
Member

Need to remove old unit tests？

What old tests? Could you provide some links?

@wu-sheng wu-sheng added this to the 8.2.0 milestone Aug 27, 2020
@wu-sheng wu-sheng added agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Aug 27, 2020
@wu-sheng
Copy link
Member

@tristaZero @terrymanu This PR was for the old ElasticJob plugin, enhancing. Could you help with checking?

@wu-sheng
Copy link
Member 

[ERROR] assertSuccess(org.apache.skywalking.apm.plugin.esjob.JobExecutorInterceptorTest)  Time elapsed: 0.011 s  <<< FAILURE!
java.lang.AssertionError: 

Expected: is "fooJob-test"
     but: was "ElasticJob/fooJob"
	at org.apache.skywalking.apm.plugin.esjob.JobExecutorInterceptorTest.assertSuccess(JobExecutorInterceptorTest.java:72)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   JobExecutorInterceptorTest.assertSuccess:72 
Expected: is "fooJob-test"
     but: was "ElasticJob/fooJob"
[ERROR]   JobExecutorInterceptorTest.assertSuccessWithoutSharding:92 
Expected: is "fooJob"
     but: was "ElasticJob/fooJob"

I don't think we should remove them. UT should be fixed.

@hailin0 hailin0 closed this Aug 27, 2020
@hailin0 hailin0 reopened this Aug 27, 2020
@wu-sheng
Copy link
Member

Why reopen? The tests fail with reasons.

terrymanu
terrymanu previously approved these changes Aug 27, 2020
View reviewed changes
Copy link
Member

@terrymanu terrymanu left a comment

Choose a reason for hiding this comment

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

The logic of changes are OK.
I suggest to merge it after fixed the problem of Chinese comment.

@hailin0
Copy link
Contributor Author

hailin0 commented Aug 27, 2020

Why reopen? The tests fail with reasons.

is misoperation

hailin0 and others added 2 commits August 27, 2020 22:01
@hailin0
Merge pull request #8 from apache/master
978cbb3 
merge
Merge branch 'elasticjob-2.x' of github.com:hailin0/skywalking
b554dab 
� Conflicts:
�	test/plugin/scenarios/elasticjob-2.x-scenario/src/main/java/org/apache/skywalking/apm/testcase/elasticjob/controller/CaseController.java
�	test/plugin/scenarios/elasticjob-2.x-scenario/src/main/java/org/apache/skywalking/apm/testcase/elasticjob/job/JobConfig.java
@wu-sheng wu-sheng merged commit afc0aef into apache:master Aug 28, 2020
vcjmhg pushed a commit to vcjmhg/skywalking that referenced this pull request Sep 7, 2020
@hailin0 @vcjmhg
Add logic-endpoint and testcase for elasticjob-2.x plugin (apache#5395)
6a1dff1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@terrymanu terrymanu terrymanu left review comments

@hanahmily hanahmily Awaiting requested review from hanahmily

Assignees
No one assigned
Labels
agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Milestone
8.2.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hailin0 @wu-sheng @terrymanu