Skip to content

Vertx plugin test re-implementation#4624

Merged
wu-sheng merged 22 commits intoapache:masterfrom
BFergerson:master
Apr 24, 2020
Merged

Vertx plugin test re-implementation#4624
wu-sheng merged 22 commits intoapache:masterfrom
BFergerson:master

Conversation

@BFergerson
Copy link
Copy Markdown
Member

Having trouble with the tests failing the health check even though I can access them when I bash into the container, like so:

$ docker exec -it vertx-web-3.x-scenario-3.7.0-local bash
root@8ceaf8677392:/usr/local/skywalking/scenario# curl http://localhost:8080/vertx-web-3-scenario/case/healthCheck
Success 

Hoping someone can take a look

@wu-sheng wu-sheng requested a review from dmsolr April 9, 2020 00:17
@wu-sheng wu-sheng added agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. test Test requirements about performance, feature or before release. labels Apr 9, 2020
@wu-sheng wu-sheng requested review from a user and rainbend April 9, 2020 00:18
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Apr 9, 2020

@dmsolr @arugal @Aderm Any of you take a look?

@BFergerson I will expect this PR hold for a while, #4599 includes some changes of plugin test due to span/ref.

Comment thread test/plugin/scenarios/vertx-web-3.x-scenario/config/expectedData.yaml Outdated
@BFergerson
Copy link
Copy Markdown
Member Author

Health check fail logs:

start container of testcase.name=vertx-web-3.x-scenario-3.7.0
WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
2020-04-09 06:15:32.231:INFO::main: Logging initialized @493ms to org.eclipse.jetty.util.log.StdErrLog
2020-04-09 06:15:32.499:INFO:oejs.Server:main: jetty-9.4.2.v20170220
2020-04-09 06:15:32.569:INFO:oejsh.ContextHandler:main: Started o.e.j.s.ServletContextHandler@d29f28{/,null,AVAILABLE}
2020-04-09 06:15:32.576:INFO:oejs.AbstractConnector:main: Started ServerConnector@1151e434{HTTP/1.1,[http/1.1]}{0.0.0.0:12800}
2020-04-09 06:15:32.577:INFO:oejs.Server:main: Started @839ms
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/run/vertx-web-3.x-scenario/libs/vertx-web-3.x-scenario.jar!/BOOT-INF/lib/logback-classic-1.1.11.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/run/vertx-web-3.x-scenario/libs/vertx-web-3.x-scenario.jar!/BOOT-INF/lib/log4j-slf4j-impl-2.6.2.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [ch.qos.logback.classic.util.ContextSelectorStaticBinder]
 health check failed!-scenario-3.7.0 url=http://localhost:8080/vertx-web-3-scenario/case/healthCheck, status=HTTP/1.1 404 Not Found
hostname: brandon-XPS-15-7590(127.0.1.1) 

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 9, 2020

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.47%. Comparing base (15f1e04) to head (b876173).
⚠️ Report is 2854 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4624      +/-   ##
============================================
+ Coverage     51.39%   51.47%   +0.07%     
- Complexity     2668     2670       +2     
============================================
  Files          1275     1274       -1     
  Lines         27659    27626      -33     
  Branches       3003     3007       +4     
============================================
+ Hits          14215    14220       +5     
+ Misses        12801    12763      -38     
  Partials        643      643              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rainbend
Copy link
Copy Markdown
Member

rainbend commented Apr 9, 2020

I started the test case locally, which seems to have something to do with curl.
image

@wu-sheng
Copy link
Copy Markdown
Member

@BFergerson New test case expected file has been used. Please take a look at other test cases. The register has been removed.

@BFergerson
Copy link
Copy Markdown
Member Author

BFergerson commented Apr 22, 2020

@wu-sheng, is there a way to specify the Netty version for the plugin test? Previously the test for 3.4.0 worked and now I'm running into eclipse-vertx/vert.x#1872. I've tried adding this dependency manually:

<dependency>
     <groupId>io.netty</groupId>
     <artifactId>netty-resolver</artifactId>
     <version>4.1.18.Final</version>
 </dependency>

@wu-sheng
Copy link
Copy Markdown
Member

@BFergerson You have the fully control of how to building the test application. We don't do anything about this.

@BFergerson
Copy link
Copy Markdown
Member Author

@wu-sheng, I believe the reason it's failing now is that I'm missing the license header in some code files? A few of the test files like (https://github.com/apache/skywalking/blob/1483e31d3938c90a6f02a7fc4a3b24e49b79bd28/test/plugin/scenarios/vertx-eventbus-3.x-scenario/src/main/java/org/apache/skywalking/apm/testcase/vertxeventbus/util/CustomMessage.java) were taken from the Vert.x example source code (https://github.com/vert-x3/vertx-examples/blob/master/core-examples/src/main/java/io/vertx/example/core/eventbus/messagecodec/util/CustomMessage.java).

The Vert.x example code is under the same license this project is so I thought it was acceptable to use the code. What's the correct way to handle this?

@wu-sheng
Copy link
Copy Markdown
Member

First, thank you for bring this up the attention, its LICENSE is important, even it is only test cases. If you are sure we need to include this(or you could rewrite the codes in other ways), we need to

According to the official release script, https://github.com/apache/skywalking/blob/master/tools/releasing/create_source_release.sh#L63-L71, test folder is included in the release source tar, so, here we should do,

  1. LICENSE of this file should be added here, https://github.com/apache/skywalking/blob/master/LICENSE#L211-L225. In this case we should indicate the file names or folder names.
  2. Exclude them from the apache rat check, https://github.com/apache/skywalking/blob/master/pom.xml#L411-L495

Personally, I prefer our own test codes, but this is not a block, choose the way you like.

Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Tests passed. @dmsolr @arugal Please review.

@wu-sheng wu-sheng closed this Apr 24, 2020
@wu-sheng wu-sheng reopened this Apr 24, 2020
@wu-sheng
Copy link
Copy Markdown
Member

I reopen this for triggering the CI.

dmsolr
dmsolr previously approved these changes Apr 24, 2020
Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

@BFergerson @dmsolr Where is the CI control file change? I can't find it.

@wu-sheng
Copy link
Copy Markdown
Member

.github/workflows/plugins-test.[0-4].yaml are the control files.

@rainbend
Copy link
Copy Markdown
Member

Health check fail logs:

start container of testcase.name=vertx-web-3.x-scenario-3.7.0
WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
2020-04-09 06:15:32.231:INFO::main: Logging initialized @493ms to org.eclipse.jetty.util.log.StdErrLog
2020-04-09 06:15:32.499:INFO:oejs.Server:main: jetty-9.4.2.v20170220
2020-04-09 06:15:32.569:INFO:oejsh.ContextHandler:main: Started o.e.j.s.ServletContextHandler@d29f28{/,null,AVAILABLE}
2020-04-09 06:15:32.576:INFO:oejs.AbstractConnector:main: Started ServerConnector@1151e434{HTTP/1.1,[http/1.1]}{0.0.0.0:12800}
2020-04-09 06:15:32.577:INFO:oejs.Server:main: Started @839ms
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/run/vertx-web-3.x-scenario/libs/vertx-web-3.x-scenario.jar!/BOOT-INF/lib/logback-classic-1.1.11.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/run/vertx-web-3.x-scenario/libs/vertx-web-3.x-scenario.jar!/BOOT-INF/lib/log4j-slf4j-impl-2.6.2.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [ch.qos.logback.classic.util.ContextSelectorStaticBinder]
 health check failed!-scenario-3.7.0 url=http://localhost:8080/vertx-web-3-scenario/case/healthCheck, status=HTTP/1.1 404 Not Found
hostname: brandon-XPS-15-7590(127.0.1.1) 

Hi @BFergerson @dmsolr
We don't seem to have solved the problem, right?

@dmsolr dmsolr dismissed their stale review April 24, 2020 07:25

Have to add Vertx testcases into github action.

@BFergerson
Copy link
Copy Markdown
Member Author

BFergerson commented Apr 24, 2020

@wu-sheng, updated CI control files.

@arugal, I solved that issue by responding to HEAD requests. I previously thought the health check was a GET request.

@wu-sheng
Copy link
Copy Markdown
Member

Vertx tests are failing, please recheck.

@BFergerson
Copy link
Copy Markdown
Member Author

Should be good to go

Copy link
Copy Markdown
Member

@rainbend rainbend left a comment

Choose a reason for hiding this comment

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

LGTM

@rainbend
Copy link
Copy Markdown
Member

https://github.com/apache/skywalking/blob/master/docs/en/guides/Plugin-test.md#configurationyml

@BFergerson We forgot to explain in the document before, could you please help to modify it :) HealthCheck is HEAD and entryService is GET.

@wu-sheng wu-sheng merged commit 776d508 into apache:master Apr 24, 2020
@wu-sheng wu-sheng mentioned this pull request Apr 24, 2020
3 tasks
@wu-sheng wu-sheng added this to the 8.0.0 milestone Apr 24, 2020
@BFergerson BFergerson mentioned this pull request Apr 29, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. test Test requirements about performance, feature or before release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants