Stackdriver integration tests #140

saturnism · 2019-08-26T21:13:37Z

Renamed existing ITStackdriverSendertest to AsyncReporterStackdriverSenderTest.java
Updated ITStackdriverSender to test against a service account
Configure the service account path via GOOGLE_APPLICATION_CREDENTIALS env var

saturnism · 2019-08-26T21:19:35Z

eak, travis encrypt-file reformatted .travis.yml. Let me know if you'd like to format it back.

also, i'm unsure whether the encrypted file will decrypt post merge.

saturnism · 2019-08-26T21:21:18Z

ok, it seems like i may have to add the encrypted key via zipkin-gcp repo.

@adriancole let me know if you are OK w/ me adding the encrypted key directly on master,
and whether reformatted .travis.yml is ok

...ackdriver/src/test/java/zipkin2/reporter/stackdriver/AsyncReporterStackdriverSenderTest.java

codefromthecrypt · 2019-08-27T01:10:05Z

I have a change in brave I've neglected for a long time so look at this closer after that. thanks for the help!

elefeint

Looks good from my end; a couple of optional comments [on comments, as it happens].

sender-stackdriver/pom.xml

sender-stackdriver/src/test/java/zipkin2/reporter/stackdriver/ITStackdriverSender.java

anuraaga · 2019-08-28T04:04:08Z

sender-stackdriver/src/test/java/zipkin2/reporter/stackdriver/ITStackdriverSender.java

-  public void setUp() {
-    server.getServiceRegistry().addService(traceService);
+  public void setUp() throws IOException {
+  	// Application Default credential is configured using the GOOGLE_APPLICATION_CREDENTIALS env var


Maybe or gcloud SDK when run locally.

in the context of this test, i think the env var is better way, the other ways should be documented in the linked page (but also, not recommended).

I'd prefer to see an assumeThat here which will ignore the test vs break the build, when remote stuff is not around. That way you can craft a better error. PS I think the build is broke not sure intended or not.

TIL :D updated to use assumeThatCode since GoogleCredentials.getApplicationDefault throws an exception that we need to catch.

.travis.yml

codefromthecrypt

looks good. one note besides looking into why this doesn't run in travis. If it is a permissions concern, odd, but let me know. maybe it is one of those things where you need to merge first. At any rate, let's use assume.. in order to keep it green.

codefromthecrypt · 2019-08-30T00:26:23Z

sender-stackdriver/src/test/java/zipkin2/reporter/stackdriver/ITStackdriverSender.java

-  public void setUp() {
-    server.getServiceRegistry().addService(traceService);
+  public void setUp() throws IOException {
+  	// Application Default credential is configured using the GOOGLE_APPLICATION_CREDENTIALS env var


I'd prefer to see an assumeThat here which will ignore the test vs break the build, when remote stuff is not around. That way you can craft a better error. PS I think the build is broke not sure intended or not.

…refreshAccessToken failed.

codefromthecrypt · 2019-08-30T03:30:29Z

something still seems busted..

saturnism · 2019-08-30T03:40:08Z

it looks like the assumption was determined not met

org.junit.AssumptionViolatedException: assumption was not met due to: 
Expecting code not to raise a throwable but caught

but it's still a failure rather than a skip.

codefromthecrypt · 2019-08-30T03:48:22Z

if me, I'd check the state it needs instead as the error message isn't 100% clear anyway. there's a slight drift risk, but then if there was, we'd have to reconfig the job

saturnism · 2019-08-30T04:15:46Z

even if i tried a simple assumeThat(System.getenv("GOOGLE_APPLICATION_CREDENTIALS")).isNotBlank();, in intelliJ it shows yellow X, but is marked as a test failure, and in maven, it's marked as failure rather than skip.

saturnism · 2019-08-30T04:21:24Z

ok i think i found the issue...

…ailing tests. during tear down, only close reporter if reporter is not null, otherwise, tearDown() will cause test failures.

codefromthecrypt · 2019-08-30T04:37:07Z

looks like you did. good detective work again @saturnism. Thanks for all the work.

Thanks for the review @elefeint and @anuraaga!

saturnism added 2 commits August 26, 2019 17:05

Integration test against a real GCP project

bcd733a

added encrypted json key

fcda077

saturnism requested a review from codefromthecrypt August 26, 2019 21:13

saturnism requested a review from elefeint August 26, 2019 21:21

saturnism added 3 commits August 26, 2019 17:25

removed un-used empty test

7f00d21

Cleaned up tests

016353a

removed redundant/bad ok check

6dbbdc0

codefromthecrypt reviewed Aug 27, 2019

View reviewed changes

...ackdriver/src/test/java/zipkin2/reporter/stackdriver/AsyncReporterStackdriverSenderTest.java Outdated Show resolved Hide resolved

saturnism added 5 commits August 26, 2019 22:17

renamed the consumer test as well

8ad5cd4

removed extra ignoreException

6474296

removed unused rule

fad3b4e

added comment on ADC

441ffc9

fixed indentation

a169bdc

elefeint approved these changes Aug 28, 2019

View reviewed changes

sender-stackdriver/pom.xml Show resolved Hide resolved

sender-stackdriver/pom.xml Outdated Show resolved Hide resolved

sender-stackdriver/src/test/java/zipkin2/reporter/stackdriver/ITStackdriverSender.java Outdated Show resolved Hide resolved

anuraaga reviewed Aug 28, 2019

View reviewed changes

saturnism added 2 commits August 28, 2019 15:34

addressed comments

64b4e91

reverted travis encrypted file for now

eba3da9

anuraaga approved these changes Aug 29, 2019

View reviewed changes

codefromthecrypt approved these changes Aug 30, 2019

View reviewed changes

saturnism added 2 commits August 29, 2019 23:07

use assumption

4f23e2f

for some reason, in travis, the getApplicationDefault succeeded, but …

eb45e98

…refreshAccessToken failed.

upgraded to junit 5 so assumption violation skips tests rather than f…

43eee38

…ailing tests. during tear down, only close reporter if reporter is not null, otherwise, tearDown() will cause test failures.

codefromthecrypt merged commit 791fd0b into openzipkin:master Aug 30, 2019

saturnism deleted the stackdriver-integration-tests branch August 30, 2019 18:42

saturnism mentioned this pull request Oct 30, 2019

Add automated testing for zipkin-gcp #130

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stackdriver integration tests #140

Stackdriver integration tests #140

saturnism commented Aug 26, 2019 •

edited

Loading

saturnism commented Aug 26, 2019 •

edited

Loading

saturnism commented Aug 26, 2019 •

edited

Loading

codefromthecrypt commented Aug 27, 2019

elefeint left a comment

anuraaga Aug 28, 2019

saturnism Aug 28, 2019

codefromthecrypt Aug 30, 2019

saturnism Aug 30, 2019

codefromthecrypt left a comment

codefromthecrypt Aug 30, 2019

codefromthecrypt commented Aug 30, 2019

saturnism commented Aug 30, 2019

codefromthecrypt commented Aug 30, 2019

saturnism commented Aug 30, 2019

saturnism commented Aug 30, 2019

codefromthecrypt commented Aug 30, 2019

Stackdriver integration tests #140

Stackdriver integration tests #140

Conversation

saturnism commented Aug 26, 2019 • edited Loading

saturnism commented Aug 26, 2019 • edited Loading

saturnism commented Aug 26, 2019 • edited Loading

codefromthecrypt commented Aug 27, 2019

elefeint left a comment

Choose a reason for hiding this comment

anuraaga Aug 28, 2019

Choose a reason for hiding this comment

saturnism Aug 28, 2019

Choose a reason for hiding this comment

codefromthecrypt Aug 30, 2019

Choose a reason for hiding this comment

saturnism Aug 30, 2019

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt Aug 30, 2019

Choose a reason for hiding this comment

codefromthecrypt commented Aug 30, 2019

saturnism commented Aug 30, 2019

codefromthecrypt commented Aug 30, 2019

saturnism commented Aug 30, 2019

saturnism commented Aug 30, 2019

codefromthecrypt commented Aug 30, 2019

saturnism commented Aug 26, 2019 •

edited

Loading

saturnism commented Aug 26, 2019 •

edited

Loading

saturnism commented Aug 26, 2019 •

edited

Loading