Provides a way to enable/disable the docker integration tests via a gradle property #1115
Conversation
jkwatson
requested review from
arminru,
bogdandrutu,
carlosalberto,
pavolloffay,
songy23 and
tylerbenson
as code owners
|
@Oberon00 will this work for you? |
Codecov Report
@@ Coverage Diff @@
## master #1115 +/- ##
=========================================
Coverage 85.68% 85.68%
Complexity 1081 1081
=========================================
Files 138 138
Lines 3974 3974
Branches 351 351
=========================================
Hits 3405 3405
- Misses 427 428 +1
+ Partials 142 141 -1
Continue to review full report at Codecov.
|
CONTRIBUTING.md
Outdated
| 1. Note: Currently, to run the full suite of tests, you'll need to be running a docker daemon. | ||
| 1. Note: Currently, to run the full suite of tests, you'll need to be running a docker daemon. If | ||
| you cannot run the docker daemon, you can disable the docker tests by setting a gradle property of | ||
| ``"disable.docker.tests"`` to true. See the gradle.properties file in the root of the project |
There was a problem hiding this comment.
I would do the opposite and disable by default, what do you think?
There was a problem hiding this comment.
I also support disabling it by default, and enabling it by default only for CircleCI.
There was a problem hiding this comment.
no strong opinion. My only concern is that we somehow get it disabled for CI and never notice.
There was a problem hiding this comment.
Personally, I like to always run them, so I'll probably just add the property to my personal gradle.properties and be fine.
There was a problem hiding this comment.
ok, reversed the logic and the name of the property (so we don't have double negatives everywhere).
There was a problem hiding this comment.
I think enabling by default but skipping the tests without error if container setup fails, unless explicitly enabled, would be best.
There was a problem hiding this comment.
So your suggestion is to always try, but only fail if the property is set to true?
jkwatson
changed the title
exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/JaegerIntegrationTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Before | ||
| public void setupJaegerExporter() { | ||
| Assume.assumeNotNull(jaeger); |
There was a problem hiding this comment.
It is confusing to have this in Before, and probably before is not needed since only one test. I would remove Before from this method and explicitly call it in the only test we have. Then the Assume is only in the test method.
There was a problem hiding this comment.
implemented. please take a look
|
Overall LGTM, no strong opinion on the specific details other than not running/enabling by default the aforementioned test cases. |
exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/JaegerIntegrationTest.java
Outdated
Show resolved
Hide resolved
| @@ -73,6 +73,10 @@ Continuous integration builds the project, runs the tests, and runs multiple | |||
| types of static analysis. | |||
|
|
|||
| 1. Note: Currently, to run the full suite of tests, you'll need to be running a docker daemon. | |||
| The tests that require docker are disabled by default. If you wish to run them, | |||
| you can enable the docker tests by setting a gradle property of | |||
| ``"enable.docker.tests"`` to true. See the gradle.properties file in the root of the project | |||
There was a problem hiding this comment.
The property name looks a bit odd to me, I'd actually expect it to be the other way round since the periods usually resemble some kind of hierarchy but that's just a nitpick 🙂
There was a problem hiding this comment.
yeah, I just made something up. I don't feel strongly about it.
Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
…er/JaegerIntegrationTest.java Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/JaegerIntegrationTest.java
Outdated
Show resolved
Hide resolved
…er/JaegerIntegrationTest.java Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
|
I'm going to merge as-is. If we want to tweak the setup, we can do that as a follow-on. |
…radle property (open-telemetry#1115) * Provides a way to disable the docker integration tests via a gradle property. * switch the property to be the opposite * add a new make target and let CI use it. * Update gradle.properties Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Update exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/JaegerIntegrationTest.java Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Update exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/JaegerIntegrationTest.java Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * simplify the test flow Co-authored-by: Armin Ruech <armin.ruech@gmail.com>
Resolves #1067