Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase. #1367

dconeybe · 2020-03-18T18:13:16Z

No description provided.

google-oss-bot · 2020-03-18T18:21:31Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`b58e788`)	Head (`81f3bb7`)	Diff
firebase-inappmessaging	apk (release)	?	3256660.00	? (?)
	aar	?	467661.00	? (?)
	apk (aggressive)	?	601315.00	? (?)
firebase-common:ktx	aar	?	5965.00	? (?)
protolite-well-known-types	apk (release)	?	561089.00	? (?)
	aar	?	1203203.00	? (?)
	apk (aggressive)	?	122378.00	? (?)
firebase-inappmessaging:ktx	aar	?	5003.00	? (?)
firebase-segmentation	apk (release)	?	1667839.00	? (?)
	aar	?	35427.00	? (?)
	apk (aggressive)	?	1017145.00	? (?)
firebase-database:ktx	aar	?	6706.00	? (?)
firebase-functions:ktx	aar	?	5844.00	? (?)
firebase-storage	apk (release)	?	976604.00	? (?)
	aar	?	119257.00	? (?)
	apk (aggressive)	?	325620.00	? (?)
firebase-common	apk (release)	?	646638.00	? (?)
	aar	?	34517.00	? (?)
	apk (aggressive)	?	82949.00	? (?)
encoders:firebase-encoders-json	aar	?	15334.00	? (?)
firebase-firestore:ktx	aar	?	7093.00	? (?)
firebase-crashlytics-ndk	apk (release)	?	1931147.00	? (?)
	aar	?	598746.00	? (?)
	apk (aggressive)	?	1166948.00	? (?)
transport:transport-api	aar	?	6439.00	? (?)
transport:transport-backend-cct	aar	?	38343.00	? (?)
firebase-inappmessaging-display:ktx	aar	?	22190.00	? (?)
firebase-database	apk (release)	?	1101570.00	? (?)
	aar	?	480458.00	? (?)
	apk (aggressive)	?	325609.00	? (?)
encoders:firebase-encoders-reflective	aar	?	7650.00	? (?)
firebase-crashlytics	apk (release)	?	1348662.00	? (?)
	aar	?	378438.00	? (?)
	apk (aggressive)	?	580727.00	? (?)
transport:transport-runtime	aar	?	122725.00	? (?)
firebase-installations	apk (release)	?	665533.00	? (?)
	aar	?	55060.00	? (?)
	apk (aggressive)	?	84597.00	? (?)
firebase-config:ktx	aar	?	6162.00	? (?)
firebase-dynamic-links	apk (release)	?	951227.00	? (?)
	aar	?	51149.00	? (?)
	apk (aggressive)	?	327459.00	? (?)
firebase-storage:ktx	aar	?	6143.00	? (?)
firebase-installations-interop	apk (release)	?	616109.00	? (?)
	aar	?	7509.00	? (?)
	apk (aggressive)	?	61714.00	? (?)
firebase-components	apk (release)	?	25749.00	? (?)
	aar	?	34495.00	? (?)
	apk (aggressive)	?	10962.00	? (?)
firebase-abt	apk (release)	?	746406.00	? (?)
	aar	?	35383.00	? (?)
	apk (aggressive)	?	85702.00	? (?)
firebase-config	apk (release)	?	1143995.00	? (?)
	aar	?	214548.00	? (?)
	apk (aggressive)	?	395834.00	? (?)
firebase-datatransport	apk (release)	?	711399.00	? (?)
	aar	?	5041.00	? (?)
	apk (aggressive)	?	116358.00	? (?)
firebase-dynamic-links:ktx	aar	?	7877.00	? (?)
firebase-inappmessaging-display	apk (release)	?	4520177.00	? (?)
	aar	?	165930.00	? (?)
	apk (aggressive)	?	1603060.00	? (?)
firebase-functions	apk (release)	?	1178560.00	? (?)
	aar	?	25859.00	? (?)
	apk (aggressive)	?	393472.00	? (?)
firebase-database-collection	apk (release)	?	912665.00	? (?)
	aar	?	34214.00	? (?)
	apk (aggressive)	?	313622.00	? (?)
firebase-firestore	apk (release)	?	3140031.00	? (?)
	aar	?	1067197.00	? (?)
	apk (aggressive)	?	443184.00	? (?)
base	apk (release)	?	8754.00	? (?)
base	apk (aggressive)	?	10678.00	? (?)

Metric Unit: byte

Test Logs

https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/1367/binary-size/1240367650991771651

codecov · 2020-03-18T18:26:27Z

Codecov Report

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

Flag	Coverage Δ	Complexity Δ
#Encoders_FirebaseEncodersJson	`?`	`?`
#Encoders_FirebaseEncodersProcessor	`?`	`?`
#Encoders_FirebaseEncodersReflective	`?`	`?`
#FirebaseAbt	`?`	`?`
#FirebaseCommon	`?`	`?`
#FirebaseCommon_DataCollectionTests	`?`	`?`
#FirebaseCommon_Ktx	`?`	`?`
#FirebaseComponents	`?`	`?`
#FirebaseConfig	`?`	`?`
#FirebaseConfig_Ktx	`?`	`?`
#FirebaseCrashlytics	`?`	`?`
#FirebaseDatabase	`?`	`?`
#FirebaseDatabaseCollection	`?`	`?`
#FirebaseDatabase_Ktx	`?`	`?`
#FirebaseDatatransport	`?`	`?`
#FirebaseDynamicLinks	`?`	`?`
#FirebaseDynamicLinks_Ktx	`?`	`?`
#FirebaseFirestore	`?`	`?`
#FirebaseFirestore_Ktx	`41.17% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#FirebaseFunctions	`?`	`?`
#FirebaseFunctions_Ktx	`?`	`?`
#FirebaseInappmessaging	`?`	`?`
#FirebaseInappmessagingDisplay	`?`	`?`
#FirebaseInappmessagingDisplay_Ktx	`?`	`?`
#FirebaseInappmessaging_Ktx	`?`	`?`
#FirebaseInstallations	`?`	`?`
#FirebaseSegmentation	`?`	`?`
#FirebaseStorage	`?`	`?`
#FirebaseStorage_Ktx	`?`	`?`
#Tools_Errorprone	`?`	`?`
#Tools_Lint	`?`	`?`
#Transport_TransportBackendCct	`?`	`?`
#Transport_TransportRuntime	`?`	`?`

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 84ab316...81f3bb7. Read the comment docs.

schmidt-sebastian · 2020-03-18T19:00:33Z

firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java

+  // -DexclusiveSpecTest=<TestName> to the Java runtime, replacing <TestName> with the name of the
+  // test to execute exclusively.  The <TestName> value is the result of appending the "itName" of
+  // the test to its "describeName", separated by a space character.
+  private static final String EXCLUSIVE_PROPERTY = "exclusiveSpecTest";


What do you think of making this a filter and allowing Regular Expressions? It should maybe add one or two lines to this PR, but make this feature much easier to use (we don't have to specify the full test name) and more flexible (we could run a group of tests).

SGTM. I'll make this change.

schmidt-sebastian · 2020-03-18T19:02:14Z

firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java

@@ -1097,6 +1106,12 @@ public void testSpecTests() throws Exception {
      parsedSpecFiles.add(new Pair<>(f.getName(), fileJSON));
    }

+    String exclusiveTestNameFromSystemProperty =
+        emptyToNull(System.getProperty(EXCLUSIVE_PROPERTY));


If you go with regular expression approach, this could be System.getProperty(EXCLUSIVE_PROPERTY, ".*"), in which case you can always apply the filter.

This won't work though because I need to be able to distinguish between the property being set or not being set. The reason is that if it is set then exclusiveMode must be set to true and if it is not then it must be left alone. By using a default value I cannot know if .* was specified explicitly or was the default value.

schmidt-sebastian · 2020-03-18T19:03:36Z

firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java

@@ -14,6 +14,7 @@

 package com.google.firebase.firestore.spec;

+import static com.google.common.base.Strings.emptyToNull;


We should try not to import more Guava dependencies, unless we want to make an exception for tests (cc @wu-hui).

In my opinion, and exception for tests makes a lot of sense since there are no downstream dependencies on the test artifacts that could result in dependency mismatch conflicts. Moreover, this class already imports com.google.common.collect.Sets from guava, so this is not a net new dependency on guava.

I am OK to import Guava in tests.

And thanks for adding this, will be really handy to try to debug a specific test.

google-oss-bot · 2020-03-18T20:09:37Z

@dconeybe: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
check-changed	`81f3bb7`	link	`/test check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

schmidt-sebastian

LGTM. If you feel like it, I think we

schmidt-sebastian · 2020-03-18T20:14:35Z

firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java

+        } else if (tags.contains(EXCLUSIVE_TAG)) {
+          runTest = true;
+        } else if (testNameFilter != null) {
+          runTest = testNameFilter.matcher(name).find();


Optional: This could just be

} else { runTest = testNameMatcher.matches(find); }

schmidt-sebastian · 2020-03-18T20:17:35Z

firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java

@@ -1097,6 +1109,16 @@ public void testSpecTests() throws Exception {
      parsedSpecFiles.add(new Pair<>(f.getName(), fileJSON));
    }

+    String testNameFilterFromSystemProperty =
+        emptyToNull(System.getProperty(TEST_FILTER_PROPERTY));
+    Pattern testNameFilter;


You should turn this into a Matcher right away, like such:

if (testNameFilterFromSystemProperty == null) { testNameFilterFromSystemProperty = ".*"; } else { exclusiveMode = true; } Matcher testNameMatcher = Pattern.compile(testNameFilterFromSystemProperty).matcher();

A Matcher object must be created with a string against which to match. So we must create a new Matcher object for each test name.
https://developer.android.com/reference/java/util/regex/Pattern#matcher(java.lang.CharSequence)

dconeybe · 2020-03-19T13:55:36Z

Thank you for the review, @schmidt-sebastian!

Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase.

2be88a3

google-oss-bot added the do-not-merge/work-in-progress label Mar 18, 2020

googlebot added the cla: yes Override cla label Mar 18, 2020

google-oss-bot added the size/S label Mar 18, 2020

dconeybe self-assigned this Mar 18, 2020

dconeybe marked this pull request as ready for review March 18, 2020 18:40

google-oss-bot removed the do-not-merge/work-in-progress label Mar 18, 2020

dconeybe requested review from google-oss-bot and schmidt-sebastian March 18, 2020 18:40

dconeybe assigned schmidt-sebastian and unassigned dconeybe Mar 18, 2020

schmidt-sebastian suggested changes Mar 18, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and dconeybe and unassigned schmidt-sebastian and wu-hui Mar 18, 2020

dconeybe assigned schmidt-sebastian and dconeybe and unassigned dconeybe and schmidt-sebastian Mar 18, 2020

Use a regex instead of an exact match for spec test filtering.

81f3bb7

google-oss-bot added size/M and removed size/S labels Mar 18, 2020

schmidt-sebastian approved these changes Mar 18, 2020

View reviewed changes

dconeybe merged commit f79cf6b into master Mar 19, 2020

dconeybe deleted the dconeybe/SpecExclusiveFlag branch March 19, 2020 13:55

dconeybe mentioned this pull request Mar 26, 2020

Add ability to selectively run spec tests via an environment variable. firebase/firebase-ios-sdk#5203

Merged

firebase locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase. #1367

Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase. #1367

Uh oh!

dconeybe commented Mar 18, 2020

Uh oh!

google-oss-bot commented Mar 18, 2020 •

edited

Loading

Uh oh!

codecov bot commented Mar 18, 2020 •

edited

Loading

Uh oh!

schmidt-sebastian Mar 18, 2020

Uh oh!

dconeybe Mar 18, 2020

Uh oh!

schmidt-sebastian Mar 18, 2020

Uh oh!

dconeybe Mar 19, 2020

Uh oh!

schmidt-sebastian Mar 18, 2020 •

edited

Loading

Uh oh!

dconeybe Mar 18, 2020

Uh oh!

wu-hui Mar 18, 2020

Uh oh!

google-oss-bot commented Mar 18, 2020

Uh oh!

schmidt-sebastian left a comment

Uh oh!

schmidt-sebastian Mar 18, 2020 •

edited

Loading

Uh oh!

schmidt-sebastian Mar 18, 2020

Uh oh!

dconeybe Mar 19, 2020

Uh oh!

dconeybe commented Mar 19, 2020

Uh oh!

Uh oh!

		@@ -14,6 +14,7 @@

		package com.google.firebase.firestore.spec;

		import static com.google.common.base.Strings.emptyToNull;

Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase. #1367

Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase. #1367

Uh oh!

Conversation

dconeybe commented Mar 18, 2020

Uh oh!

google-oss-bot commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

Test Logs

Uh oh!

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

Uh oh!

Codecov Report

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!

schmidt-sebastian Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

google-oss-bot commented Mar 18, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dconeybe commented Mar 19, 2020

Uh oh!

Uh oh!

google-oss-bot commented Mar 18, 2020 •

edited

Loading

codecov bot commented Mar 18, 2020 •

edited

Loading

schmidt-sebastian Mar 18, 2020 •

edited

Loading

schmidt-sebastian Mar 18, 2020 •

edited

Loading