Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

pubsub emulator rule moved to a separate project #2352

Closed
wants to merge 3 commits into from

Conversation

dmitry-s
Copy link
Contributor

fixes #2351

cc/ @elefeint

@dmitry-s dmitry-s requested a review from meltsufin April 30, 2020 20:03
@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #2352 into master will decrease coverage by 7.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2352      +/-   ##
============================================
- Coverage     81.10%   74.04%   -7.07%     
+ Complexity     2315     2098     -217     
============================================
  Files           259      259              
  Lines          7563     7563              
  Branches        785      785              
============================================
- Hits           6134     5600     -534     
- Misses         1103     1603     +500     
- Partials        326      360      +34     
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 74.04% <ø> (ø) 2098.00 <ø> (ø)
Impacted Files Coverage Δ Complexity Δ
...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%)
...figure/config/GcpConfigBootstrapConfiguration.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...epository/config/SpannerRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...ository/config/DatastoreRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...restore/GcpFirestoreEmulatorAutoConfiguration.java 0.00% <0.00%> (-84.85%) 0.00% <0.00%> (-4.00%)
...ramework/cloud/gcp/vision/DocumentOcrTemplate.java 17.64% <0.00%> (-73.53%) 4.00% <0.00%> (-5.00%)
...work/cloud/gcp/bigquery/core/BigQueryTemplate.java 0.00% <0.00%> (-71.70%) 0.00% <0.00%> (-8.00%)
...tegration/outbound/BigQueryFileMessageHandler.java 0.00% <0.00%> (-56.15%) 0.00% <0.00%> (-10.00%)
... and 51 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 b01e784...10cdf4c. Read the comment docs.

pom.xml Outdated
@@ -45,6 +45,7 @@
<module>spring-cloud-gcp-bigquery</module>
<module>spring-cloud-gcp-security-firebase</module>
<module>spring-cloud-gcp-secretmanager</module>
<module>spring-cloud-gcp-test-support</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename to spring-cloud-gcp-test.

@dmitry-s dmitry-s requested a review from meltsufin May 4, 2020 23:01
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 4, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 6 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@meltsufin meltsufin requested a review from elefeint May 5, 2020 13:37
<dependencies>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

test scope?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not in this case -- the test project itself is meant to be imported as test scope, but the rule extends a JUnit class, so it needs to be compile, not test scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should consider making these utilities independent of JUnit. People use different test frameworks.

Copy link
Contributor

Choose a reason for hiding this comment

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

make it generic so people can reuse it sounds good. Can you take into account that may some people will be able to contribute creating rules for junit4 or extension for junit5 specifically? Those can be issues with ideal for contribution or help wanted. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That a good idea! We should do that.

<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-jcl</artifactId>
<scope>compile</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

Compile scope is the default, you don't have to specify it.

@@ -87,7 +86,7 @@ public PubSubEmulator() {
@Override
protected void before() throws IOException, InterruptedException {

assumeTrue("PubSubEmulator rule disabled. Please enable with -Dit.pubsub-emulator.", this.enableTests);
Assume.assumeTrue("PubSubEmulator rule disabled. Please enable with -Dit.pubsub-emulator.", this.enableTests);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that enabling it with -Dit.pubsub-emulator makes sense when you're offering it as a generic test utility. That flag was specific for our project.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an interesting question then to test whether the rule initialization or a test's @BeforeClass will run first. We would not want to do emulator startup if the tests themselves are disabled.

If there is possibility of the rule running first, perhaps we can just document this flag well?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also try not to implement JUnit test rule.
That's what we ended up doing for Spring Cloud Function here.

@@ -14,7 +14,7 @@
* limitations under the License.
*/

package org.springframework.cloud.gcp.stream.binder.pubsub;
package org.springframework.cloud.gcp.test;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
package org.springframework.cloud.gcp.test;
package org.springframework.cloud.gcp.test.pubsub;

@dmitry-s
Copy link
Contributor Author

dmitry-s commented May 7, 2020

superseded by #2357

@dmitry-s dmitry-s closed this May 7, 2020
@meltsufin meltsufin deleted the pubsub-emulator-rule branch May 7, 2020 21:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make PubSub emulator rule public
4 participants