Test support module by dmitry-s · Pull Request #2357 · spring-attic/spring-cloud-gcp

dmitry-s · 2020-05-07T21:04:13Z

Add test module and introduce spanner emulator helper tools.

codecov · 2020-05-07T21:26:15Z

Codecov Report

Merging #2357 into master will decrease coverage by 7.75%.
The diff coverage is 50.28%.

@@             Coverage Diff              @@
##             master    #2357      +/-   ##
============================================
- Coverage     81.10%   73.35%   -7.76%     
+ Complexity     2315     2124     -191     
============================================
  Files           259      267       +8     
  Lines          7563     7750     +187     
  Branches        785      797      +12     
============================================
- Hits           6134     5685     -449     
- Misses         1103     1693     +590     
- Partials        326      372      +46

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`73.35% <50.28%> (-0.69%)`	`2124.00 <28.00> (+26.00)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
...ework/cloud/gcp/test/EmulatorRuntimeException.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...mework/cloud/gcp/test/spanner/SpannerEmulator.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...st/spanner/SpannerEmulatorSpringConfiguration.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...springframework/cloud/gcp/test/EmulatorDriver.java	`57.72% <57.72%> (ø)`	`19.00 <19.00> (?)`
...ramework/cloud/gcp/test/pubsub/PubSubEmulator.java	`61.53% <61.53%> (ø)`	`4.00 <4.00> (?)`
...g/springframework/cloud/gcp/test/EmulatorRule.java	`87.50% <87.50%> (ø)`	`3.00 <3.00> (?)`
...a/org/springframework/cloud/gcp/test/Emulator.java	`100.00% <100.00%> (ø)`	`2.00 <2.00> (?)`
...gcp/secretmanager/SecretManagerPropertySource.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-4.00%)`
...a/spanner/repository/query/SpannerQueryMethod.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-6.00%)`
...retmanager/SecretManagerPropertySourceLocator.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
... and 70 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 7582692...10a1019. Read the comment docs.

meltsufin

Please try to address the Sonar findings where it makes sense.

meltsufin · 2020-05-07T21:56:56Z

...ng-cloud-gcp-test/src/main/java/org/springframework/cloud/gcp/test/PubSubEmulatorHelper.java

+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
+public class PubSubEmulatorHelper extends AbstractEmulatorHelper {


Can we please refactor this not to use inheritance?
You can try turning all of the abstract methods of AbstractEmulatorHelper into an interface that each emulator would implement and then a final EmulatorHelper can be customized by.
Alternatively, you can make EmulatorHelper a generic helper that any implementation of an emulator can re-use.

Why though? Sincerely curious; this seems like a reasonable case for inheritance.

I'm sure if you search for "inheritance is evil", lots of reasons will pop up.
From Effective Java 3rd edition:
See:
Item 18: Favor composition over inheritance
Item 19: Design and document for inheritance or else prohibit it
Item 20: Prefer interfaces to abstract classes

I don't do religious wars.

Do you think it's the same as tabs vs spaces? 😄

Yep. Each side can be justified with rational arguments, and yet, the disagreement lives on. We'll have to do a PR converting Spring tabs into Google spaces soon...

eddumelendez · 2020-05-08T18:14:11Z

...st/java/org/springframework/cloud/gcp/data/spanner/core/it/SpannerTemplateEmulatorTests.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2017-2018 the original author or authors.


update copyright's year

eddumelendez · 2020-05-08T18:15:43Z

...ng-cloud-gcp-test/src/main/java/org/springframework/cloud/gcp/test/PubSubEmulatorHelper.java

+public class PubSubEmulatorHelper extends AbstractEmulatorHelper {
+	private static final Log LOGGER = LogFactory.getLog(PubSubEmulatorHelper.class);
+
+	String getGatingPropertyName() {


not sure if this should be provided

docs/src/main/asciidoc/test.adoc

meltsufin · 2020-05-14T21:17:24Z

...d-gcp-test/src/test/java/org/springframework/cloud/gcp/test/PubSubTemplateEmulatorTests.java

+
+	@BeforeClass
+	public static void enableTests() {
+		Assumptions.assumeThat(System.getProperty("it.emulator"))


It should be -Dit.pubsub-emulator.

Let's keep it like this until we update Travis CI configuration. Otherwise CI would fail.

meltsufin · 2020-05-14T21:21:12Z

...d-gcp-test/src/test/java/org/springframework/cloud/gcp/test/PubSubTemplateEmulatorTests.java

+import org.springframework.boot.test.context.runner.ApplicationContextRunner;
+import org.springframework.cloud.gcp.autoconfigure.core.GcpContextAutoConfiguration;
+import org.springframework.cloud.gcp.autoconfigure.pubsub.GcpPubSubAutoConfiguration;
+import org.springframework.cloud.gcp.autoconfigure.pubsub.GcpPubSubEmulatorAutoConfiguration;


Should we deprecate GcpPubSubEmulatorAutoConfiguration and move it to the test module?

We should still support the manual path where someone starts an emulator themselves, and then runs an application relying on our pubsub starter, providing an emulator property.

meltsufin · 2020-05-14T21:22:34Z

...d-gcp-test/src/test/java/org/springframework/cloud/gcp/test/PubSubTemplateEmulatorTests.java

+	}
+
+	@Test
+	public void testCreatePublishPullNextAndDelete() {


How are you verifying that it's actually the emulator that you are using vs. the real service?
Maybe check the host or something?

It's done inside the context below -- assertThat(transportChannelProvider.getTransportChannel().toString()).contains("target=dns:///localhost:8085");

meltsufin · 2020-05-14T21:23:50Z

...-gcp-test/src/test/java/org/springframework/cloud/gcp/test/SpannerTemplateEmulatorTests.java

+
+	@BeforeClass
+	public static void checkToRun() {
+		Assumptions.assumeThat(System.getProperty("it.emulator"))


It should be -Dit.spanner-emulator.
Also, can we update Travis CI config to actually run this test? What would it take?

We already install gcloud, so installing emulators would be more of the same. Can be a follow up PR to update Travis config -- Dmitry wanted to add it right away, but I remember how annoying testing travis config is (multiple tries, the only way to test is by committing), so I asked for a separate minimal Travis config PR.

meltsufin · 2020-05-14T21:26:52Z

...loud-gcp-test/src/test/java/org/springframework/cloud/gcp/test/SpannerTestConfiguration.java

+@Configuration
+@EnableTransactionManagement
+@EnableSpannerRepositories
+public class SpannerTestConfiguration {


Can we make it an auto-configuration like we have for Pub/Sub?
I imagine some folks will want to use the emulator to test the whole application, rather than just run the tests.

Eddú did that in #2356, but we've created this PR before that. Perhaps a follow-up PR to switch over to official configuration?

Co-authored-by: Mike Eltsufin <meltsufin@google.com>

elefeint

This turned out really well!

I wonder if we should create a package for "emulators", since we may want to add non-emulator test support in the future. But this can be done in the new repo.

elefeint · 2020-05-15T16:13:06Z

docs/src/main/asciidoc/test.adoc

+}
+----
+
+NOTE: The class rule doesn't work for `SpannerEmulator`.


Should we mention the workaround of setting up a custom Spanner bean and setting its destroy method to ""?

It looks like implementation details to me. @meltsufin what do you think?

elefeint · 2020-05-15T18:40:27Z

.../test/java/org/springframework/cloud/gcp/data/spanner/test/IntegrationTestConfiguration.java

 	}

 	@Bean
+	@ConditionalOnMissingBean


Wait, remove the bean definitions entirely? There are several integration tests that go against real spanner.

elefeint · 2020-05-15T18:42:19Z

spring-cloud-gcp-test/pom.xml

+
+		<dependency>
+			<groupId>org.springframework</groupId>
+			<artifactId>spring-jcl</artifactId>


Should this be optional, too? And move under the Spring Dependencies comment?

elefeint · 2020-05-15T18:43:44Z

spring-cloud-gcp-test/src/main/java/org/springframework/cloud/gcp/test/Emulator.java

+
+	/**
+	 * The list of command fragments that match the emulator processes to be killed.
+	 * @param hostPort THe emulator host-port.


spring-cloud-gcp-test/src/main/java/org/springframework/cloud/gcp/test/Emulator.java

...main/java/org/springframework/cloud/gcp/test/spanner/SpannerEmulatorSpringConfiguration.java

elefeint · 2020-05-15T19:24:49Z

...d-gcp-test/src/test/java/org/springframework/cloud/gcp/test/PubSubTemplateEmulatorTests.java

+import org.springframework.boot.test.context.runner.ApplicationContextRunner;
+import org.springframework.cloud.gcp.autoconfigure.core.GcpContextAutoConfiguration;
+import org.springframework.cloud.gcp.autoconfigure.pubsub.GcpPubSubAutoConfiguration;
+import org.springframework.cloud.gcp.autoconfigure.pubsub.GcpPubSubEmulatorAutoConfiguration;


We should still support the manual path where someone starts an emulator themselves, and then runs an application relying on our pubsub starter, providing an emulator property.

elefeint · 2020-05-15T19:26:21Z

...d-gcp-test/src/test/java/org/springframework/cloud/gcp/test/PubSubTemplateEmulatorTests.java

+	}
+
+	@Test
+	public void testCreatePublishPullNextAndDelete() {


It's done inside the context below -- assertThat(transportChannelProvider.getTransportChannel().toString()).contains("target=dns:///localhost:8085");

elefeint · 2020-05-15T19:27:45Z

...-gcp-test/src/test/java/org/springframework/cloud/gcp/test/SpannerTemplateEmulatorTests.java

+
+	@BeforeClass
+	public static void checkToRun() {
+		Assumptions.assumeThat(System.getProperty("it.emulator"))


We already install gcloud, so installing emulators would be more of the same. Can be a follow up PR to update Travis config -- Dmitry wanted to add it right away, but I remember how annoying testing travis config is (multiple tries, the only way to test is by committing), so I asked for a separate minimal Travis config PR.

elefeint · 2020-05-15T19:35:38Z

...loud-gcp-test/src/test/java/org/springframework/cloud/gcp/test/SpannerTestConfiguration.java

+@Configuration
+@EnableTransactionManagement
+@EnableSpannerRepositories
+public class SpannerTestConfiguration {


Eddú did that in #2356, but we've created this PR before that. Perhaps a follow-up PR to switch over to official configuration?

Co-authored-by: Elena Felder <41136058+elefeint@users.noreply.github.com>

…ud-gcp into spanner-emulator

sonarqubecloud · 2020-05-18T21:38:07Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
3 Code Smells

53.1% Coverage
0.0% Duplication

dmitry-s · 2020-10-05T18:38:22Z

awaiting for redesign

ttomsu · 2020-10-12T12:56:00Z

Converted PR to draft while we wait.

dmitry-s and others added 11 commits May 6, 2020 15:11

GcpEmulator parent class

af58b0c

create GcpEmulator parent class

616fd6c

run all necessary setup, working test

c97d2a5

refactored to allow killing arbitrary commands

e760570

refactor to be independent from junit and spring

ddd4fb7

break travis config with a TODO

6e9f411

rename test module

2a51055

fixed incorrect default project ID making it into spanner object

2ac9f78

unbreak travis

9f848dc

PR comments

4a80876

add docs and clean up the code

ef720e0

dmitry-s requested review from elefeint and meltsufin May 7, 2020 21:04

dmitry-s mentioned this pull request May 7, 2020

pubsub emulator rule moved to a separate project #2352

Closed

meltsufin suggested changes May 7, 2020

View reviewed changes

sonar feedback

fe11788

eddumelendez reviewed May 8, 2020

View reviewed changes

meltsufin and others added 2 commits May 8, 2020 15:30

Massive refactoring with Dmitry and Elena

7b119e7

add gating properties and improve docs

8af2f8a

meltsufin mentioned this pull request May 12, 2020

Add annotation which bring support to test against spanner #2281

Closed

dmitry-s added 3 commits May 12, 2020 18:35

add tests

9d0f694

address sonarqube issues

d043c0e

address sonarqube issues

946914a

meltsufin suggested changes May 14, 2020

View reviewed changes

dmitry-s requested a review from meltsufin May 14, 2020 21:46

dmitry-s and others added 2 commits May 14, 2020 17:54

Apply suggestions from code review

df4c69c

Co-authored-by: Mike Eltsufin <meltsufin@google.com>

PR comments

aa31b9d

elefeint suggested changes May 15, 2020

View reviewed changes

dmitry-s and others added 4 commits May 15, 2020 16:23

Apply suggestions from code review

dab420e

Co-authored-by: Elena Felder <41136058+elefeint@users.noreply.github.com>

PR comments

4d9fd10

Merge branch 'spanner-emulator' of github.com:spring-cloud/spring-clo…

e5e49b7

…ud-gcp into spanner-emulator

PR comments

36cbc62

elefeint previously approved these changes May 15, 2020

View reviewed changes

remove unused imports

10a1019

dmitry-s dismissed elefeint’s stale review via 10a1019 May 18, 2020 21:28

meltsufin requested a review from elefeint May 28, 2020 13:58

eddumelendez mentioned this pull request Jun 5, 2020

Add DataSpannerTest annotation #2416

Closed

ttomsu marked this pull request as draft October 12, 2020 12:55

meltsufin mentioned this pull request Feb 22, 2021

Allow starting Pub/Sub emulator for local development and integration testing GoogleCloudPlatform/spring-cloud-gcp#333

Closed

		@@ -0,0 +1,48 @@
		/*
		* Copyright 2017-2018 the original author or authors.

Conversation

dmitry-s commented May 7, 2020

Uh oh!

codecov bot commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

meltsufin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elefeint left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 18, 2020

Uh oh!

dmitry-s commented Oct 5, 2020

Uh oh!

ttomsu commented Oct 12, 2020

Uh oh!

codecov bot commented May 7, 2020 •

edited

Loading