Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 19, 2020
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2be88a3
Add support for -DexclusiveSpecTest=<TestName> to SpecTestCase.
dconeybe Mar 10, 2020
81f3bb7
Use a regex instead of an exact match for spec test filtering.
dconeybe Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.firebase.firestore.spec;

import static com.google.common.base.Strings.emptyToNull;
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am OK to import Guava in tests.

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

import static com.google.firebase.firestore.TestUtil.waitFor;
import static com.google.firebase.firestore.testutil.TestUtil.ARBITRARY_SEQUENCE_NUMBER;
import static com.google.firebase.firestore.testutil.TestUtil.deleteMutation;
Expand Down Expand Up @@ -93,6 +94,7 @@
import java.util.Map;
import java.util.Set;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.json.JSONArray;
Expand Down Expand Up @@ -132,6 +134,16 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
// this tag and they'll all be run (but all others won't).
private static final String EXCLUSIVE_TAG = "exclusive";

// The name of a Java system property ({@link System#getProperty(String)}) whose value is a filter
// that specifies which tests to execute. The value of this property is a regular expression that
// is matched against the name of each test. Using this property is an alternative to setting the
// {@link #EXCLUSIVE_TAG} tag, which requires modifying the JSON file. To use this property,
// specify -DspecTestFilter=<Regex> to the Java runtime, replacing <Regex> with a regular
// expression; a test will be executed if and only if its name matches this regular expression.
// In this context, a test's "name" is the result of appending its "itName" to its "describeName",
// separated by a space character.
private static final String TEST_FILTER_PROPERTY = "specTestFilter";

// Tags on tests that should be excluded from execution, useful to allow the platforms to
// temporarily diverge or for features that are designed to be platform specific (such as
// 'multi-client').
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

if (testNameFilterFromSystemProperty == null) {
testNameFilter = null;
} else {
exclusiveMode = true;
testNameFilter = Pattern.compile(testNameFilterFromSystemProperty);
}

for (Pair<String, JSONObject> parsedSpecFile : parsedSpecFiles) {
String fileName = parsedSpecFile.first;
JSONObject fileJSON = parsedSpecFile.second;
Expand All @@ -1115,7 +1137,19 @@ public void testSpecTests() throws Exception {
JSONArray steps = testJSON.getJSONArray("steps");
Set<String> tags = getTestTags(testJSON);

boolean runTest = shouldRunTest(tags) && (!exclusiveMode || tags.contains(EXCLUSIVE_TAG));
boolean runTest;
if (!shouldRunTest(tags)) {
runTest = false;
} else if (!exclusiveMode) {
runTest = true;
} else if (tags.contains(EXCLUSIVE_TAG)) {
runTest = true;
} else if (testNameFilter != null) {
runTest = testNameFilter.matcher(name).find();
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Optional: This could just be

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

} else {
runTest = false;
}

boolean measureRuntime = tags.contains(BENCHMARK_TAG);
if (runTest) {
long start = System.currentTimeMillis();
Expand Down