adding custom logging test + local runtimes-common integration test #134

balopat · 2017-08-17T14:58:42Z

This the first half of Test the Integration with Cloud Logging #5 enabling custom logging tests. The standard logging tests are going to be in the next PR.
- Important to note: improvements on custom logging tests runtimes-common#308 is a prerequisite for this, otherwise the driver is not able to pick up the project id from the docker container (as it's using gcloud for that which is currently not setup in the docker container with the right information). However, for now I published my branch version to the java-runtimes-test docker registry that can serve as our version of the driver until we negotiate the PR with @nkubala.
- closes Setup local integration test with the runtimes-common integration testing #126
- closes Structure tests fails (maven property missing) with Maven 3.0.5 #145
- closes Use local container builder for running integration tests locally #140

TODO left:

fix local runtimes-common integration tests with local cloud builder (if possible)
fix logging test timeout issue (recently came up)
final review and refactor (opened new ticket for a larger redesign: Reduce duplication across test scripts and reuse code across java runtimes tests #148)

aslo · 2017-08-17T19:20:40Z

scripts/local_integration_test.sh

+docker build -t $APP_IMAGE . || gcloud docker -- build -t $APP_IMAGE . || usage "Error building test-app image from base image \"${imageUnderTest}\", please make sure it exists!"
+
+
+if [[ "$WITH_GCP" ]]; then


I don't think it makes sense to me to add these integration tests to the local_integration_test.sh script. I think we originally added this script in order to test things that were outside the scope of the runtimes_common integraion testing framework (namely, container shutdown hooks).

I think if we wanted to make it easier to run the runtimes_common tests, a better way to do it would be to just simulate a cloudbuild execution locally, using the container-builder-local emulator that was just released as part of the Cloud SDK (see release notes).

oooo, I'll test out this container-build-local, thanks for the suggestion!
I created a separate script first btw, but then I figured that they were so similar that I integrated it back into the local_integration_test.sh. It's really handy to have as part of the workflow. But if container-build-local can do the magic, I'll have a look at that as that could remove the duplication around the arguments of the runtimes-common driver for example (one place here, the other place is the yaml file).

I will do this in #126 though, and then separate it out from this pull request.

aslo · 2017-08-17T19:22:55Z

test-application/src/main/java/com/google/cloud/runtimes/config/MockGCPConfiguration.java

+import static org.mockito.Mockito.mock;
+
+@Configuration
+@Profile("mock-gcp")


I'm skeptical about having these separate profiles for gcp and mock-gcp. Is it possible to write a single test app implementation, and have the environment in which the test app runs handle the responsibility of passing in google credentials?

One of the reasons I introduced this was that without mock-gcp - the local_integration_test.sh (and I mean the shutdown log testing too, not just the local runtime-commons test) would need to pass in the GCP creds, otherwise the app doesn't start up. Is it okay to expect a valid GCP setup for the user in the local_integration_test.sh?

Maybe the local_integration_test.sh script could check for the presence of the GOOGLE_CREDENTIALS environment variable, and either fail or warn the user if it looks like the environment wasn't properly set up. What do you think about that?

It's doable, the only thing I have against that is that this decision will force someone to setup a service account even when they start their development...which might not be a too strong issue but we'll have to document it then.

@meltsufin what do you think?

Can we just not invoke this logging test when running locally? Another option would be to have a separate test-app that doesn't depend on GCP for local testing. Otherwise, I don't really mind the mock-gcp profile.

The problem is that I am using Spring to inject the services, so when the beans get created, they need the credentials and thus the application fails to start without the creds - that's why I went down the road to create the two profiles. I just had an idea that I'll check out: I should try to make the components @Lazy.
Re: skipping the GCP related tests locally: if we combine @aslo's idea of separating the runtimes-common tests out into a separate script, then this will naturally skip calling them in the local integration tests.
So, if I can make @Lazy work, we have a "single test app implementation" (without need for profiles), the app starts up and we'll have the runtimes-common tests separated, which might need the GCP creds.

meltsufin · 2017-08-21T16:01:35Z

test-application/src/main/java/com/google/cloud/runtimes/LoggingTestController.java

+import com.google.cloud.logging.Logging;
+import com.google.cloud.logging.Payload;
+import com.google.cloud.logging.Severity;
+import com.google.cloud.runtimes.stackdriver.StackDriverMonitoringService;


Unused import

meltsufin · 2017-08-21T16:01:42Z

test-application/src/main/java/com/google/cloud/runtimes/LoggingTestController.java

+import org.springframework.web.bind.annotation.RequestBody;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RestController;
+import sun.util.logging.resources.logging;


Unusued import

meltsufin · 2017-08-21T16:02:12Z

test-application/src/main/java/com/google/cloud/runtimes/LoggingTestController.java

+    }
+
+    @RequestMapping(path = "/logging_custom", method = POST)
+    public String handleMonitoringRequest(@RequestBody LoggingTestRequest loggingTestRequest) throws IOException, InterruptedException {


handleLoggingRequest?

meltsufin · 2017-08-21T16:07:16Z

test-application/src/main/java/com/google/cloud/runtimes/LoggingTestController.java

+                .setResource(MonitoredResource.newBuilder("global").build())
+                .build();
+        entries.add(entry);
+        logging.write(entries);


Instead of using the Logging client lib directly, we should be able to use the JUL appender. Can you also look at #72 that has been put on the back-burner, and reconcile it with what you're doing in this PR? Ideally, we would like to close out #72 as well, but I think there are some unresolved complications there with setting up JUL.

I think that the Custom Logging Test should use the client library and make an explicit call to Stackdriver Logging (by creating only one LogEntry), whereas the Standard Logging Test should redirect the output of JUL to Stackdriver.

I agree with @cassand - I was planning to use JUL for standard logging tests and Logging library directly for custom, just so that we have both covered.
However, we are free to decide what we want to do, as we can setup custom logging with JUL too.

If you're okay with that, I will review #72 in the context of the Standard Logging test (I just added a new, separate issue for it: #139)

I'm OK with Damien's suggestion.

meltsufin · 2017-08-21T16:08:36Z

test-application/src/main/java/com/google/cloud/runtimes/config/GCPConfiguration.java

+
+@Configuration
+@Profile("gcp")
+public class GCPConfiguration {


GcpConfiguration

meltsufin · 2017-08-21T16:08:56Z

test-application/src/main/java/com/google/cloud/runtimes/config/MockGCPConfiguration.java

+
+@Configuration
+@Profile("mock-gcp")
+public class MockGCPConfiguration {


MockGcpConfiguration

meltsufin · 2017-08-21T16:13:15Z

test-application/src/main/java/com/google/cloud/runtimes/config/MockGCPConfiguration.java

+import static org.mockito.Mockito.mock;
+
+@Configuration
+@Profile("mock-gcp")


Can we just not invoke this logging test when running locally? Another option would be to have a separate test-app that doesn't depend on GCP for local testing. Otherwise, I don't really mind the mock-gcp profile.

…resolution

balopat · 2017-09-07T15:44:26Z

@aslo @meltsufin can you please review again this PR? I addressed all of your concerns:

@aslo: removed the separate MockGCP profile and introduced container-builder-local for local testing
@meltsufin: cleanup of unused imports and method rename (handleLoggingRequest) is done

Thanks!

meltsufin

I think there is also a fair amount of code duplication between the two local_*.sh scripts. I wonder if there is a way to factor out that duplication. Perhaps for a different PR though.

meltsufin · 2017-09-07T18:51:03Z

test-application/pom.xml

+    </dependency>
+    <dependency>
+      <groupId>com.google.api</groupId>
+      <artifactId>gax-grpc</artifactId>


Is this (and the one above) not a transitive dependency of google-cloud-logging?

good catch - let me double check & I will clarify in comments.

meltsufin · 2017-09-07T18:51:31Z

test-application/pom.xml

+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-core</artifactId>
+      <version>2.8.47</version>


<scope>test</scope>

meltsufin · 2017-09-07T19:09:53Z

scripts/local_runtimes_common_integration_test.sh

+  exit 1
+fi
+
+if [[ -z ${GOOGLE_APPLICATION_CREDENTIALS} ]]; then


The readme instructions don't mention GOOGLE_APPLICATION_CREDENTIALS.
Can we just leverage gcloud for the default credentials?

meltsufin · 2017-09-07T19:11:16Z

scripts/local_runtimes_common_integration_test.sh

+
+docker rm -f $CONTAINER || echo "Integration-test-app container is not running, ready to start a new instance."
+
+# run app container locally to test shutdown logging


You're not really doing a shutdown logging test here.

…eCloudPlatform/openjdk-runtime into feature/5_logging_integration_test

…ead of GOOGLE_APPLICATION_CREDENTIALS

…me into common-test-application * 'master' of github.com:GoogleCloudPlatform/openjdk-runtime: adding custom logging test + local runtimes-common integration test (#134)

adding custom logging test + local runtimes-common integration test

991e696

balopat requested review from aslo and meltsufin August 17, 2017 15:10

aslo suggested changes Aug 17, 2017

View reviewed changes

meltsufin requested changes Aug 21, 2017

View reviewed changes

balopat added 4 commits August 22, 2017 11:17

organize imports + namings

d1ea344

changed test-app to lazy init + local-cloud-builder

d2aa233

Merge branch 'master' into feature/5_logging_integration_test

bf08154

DEVELOPING instructions updated + enforcer plugin fixed for property …

5e98cd7

…resolution

balopat added 2 commits September 7, 2017 11:45

cleanup

649b8d2

cleanup

b05354e

aslo approved these changes Sep 7, 2017

View reviewed changes

meltsufin requested changes Sep 7, 2017

View reviewed changes

balopat and others added 4 commits September 7, 2017 16:15

Add shutdown tests back to maven execution (#149)

0cffb44

Merge branch 'feature/5_logging_integration_test' of github.com:Googl…

ff6b6cf

…eCloudPlatform/openjdk-runtime into feature/5_logging_integration_test

cleanup + adding automated application-default credentials logic inst…

e3eff4c

…ead of GOOGLE_APPLICATION_CREDENTIALS

cleaning up unnecessary dependencies

1654ef9

meltsufin approved these changes Sep 7, 2017

View reviewed changes

balopat merged commit ad4b0a8 into master Sep 7, 2017

balopat deleted the feature/5_logging_integration_test branch September 7, 2017 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding custom logging test + local runtimes-common integration test #134

adding custom logging test + local runtimes-common integration test #134

balopat commented Aug 17, 2017 •

edited

Loading

aslo Aug 17, 2017

balopat Aug 17, 2017

balopat Aug 22, 2017

aslo Aug 17, 2017

balopat Aug 17, 2017 •

edited

Loading

aslo Aug 17, 2017

balopat Aug 17, 2017

balopat Aug 17, 2017

meltsufin Aug 21, 2017

balopat Aug 22, 2017

meltsufin Aug 21, 2017

meltsufin Aug 21, 2017

meltsufin Aug 21, 2017

meltsufin Aug 21, 2017

cassand Aug 21, 2017

balopat Aug 22, 2017 •

edited

Loading

meltsufin Aug 22, 2017

meltsufin Aug 21, 2017

meltsufin Aug 21, 2017

meltsufin Aug 21, 2017

balopat commented Sep 7, 2017

meltsufin left a comment

meltsufin Sep 7, 2017

balopat Sep 7, 2017

meltsufin Sep 7, 2017

meltsufin Sep 7, 2017

meltsufin Sep 7, 2017

		docker build -t $APP_IMAGE . \|\| gcloud docker -- build -t $APP_IMAGE . \|\| usage "Error building test-app image from base image \"${imageUnderTest}\", please make sure it exists!"


		if [[ "$WITH_GCP" ]]; then


		docker rm -f $CONTAINER \|\| echo "Integration-test-app container is not running, ready to start a new instance."

		# run app container locally to test shutdown logging

adding custom logging test + local runtimes-common integration test #134

adding custom logging test + local runtimes-common integration test #134

Conversation

balopat commented Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balopat Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balopat Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balopat commented Sep 7, 2017

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balopat commented Aug 17, 2017 •

edited

Loading

balopat Aug 17, 2017 •

edited

Loading

balopat Aug 22, 2017 •

edited

Loading