Skip to content

[realppl 4] Port spec tests for realtime ppl #7352

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

Open
wants to merge 1 commit into
base: wuandy/RealPpl_3
Choose a base branch
from
Open

[realppl 4] Port spec tests for realtime ppl #7352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions ...se-firestore/src/test/java/com/google/firebase/firestore/spec/MemoryPipelineSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.firebase.firestore.spec;

public class MemoryPipelineSpecTest extends MemorySpecTest {
public MemoryPipelineSpecTest() {
usePipelineMode = true;
}
}
6 changes: 3 additions & 3 deletions firebase-firestore/src/test/java/com/google/firebase/firestore/spec/QueryEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@

import androidx.annotation.Nullable;
import com.google.firebase.firestore.FirebaseFirestoreException;
import com.google.firebase.firestore.core.Query;
import com.google.firebase.firestore.core.QueryOrPipeline;
import com.google.firebase.firestore.core.ViewSnapshot;

/** Object that contains exactly one of either a view snapshot or an error for the given query. */
public class QueryEvent {
public Query query;
public QueryOrPipeline queryOrPipeline;
public @Nullable ViewSnapshot view;
public @Nullable FirebaseFirestoreException error;

@Override
public String toString() {
return "QueryEvent(" + query + ", " + view + ", " + error + ")";
return "QueryEvent(" + queryOrPipeline + ", " + view + ", " + error + ")";
}
}
21 changes: 21 additions & 0 deletions ...se-firestore/src/test/java/com/google/firebase/firestore/spec/SQLitePipelineSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.firebase.firestore.spec;

public class SQLitePipelineSpecTest extends SQLiteSpecTest {
public SQLitePipelineSpecTest() {
usePipelineMode = true;
}
}
85 changes: 76 additions & 9 deletions 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 @@ -41,8 +41,10 @@
import com.google.firebase.firestore.EventListener;
import com.google.firebase.firestore.FirebaseFirestore;
import com.google.firebase.firestore.FirebaseFirestoreException;
import com.google.firebase.firestore.FirebaseFirestoreIntegrationTestFactory;
import com.google.firebase.firestore.ListenSource;
import com.google.firebase.firestore.LoadBundleTask;
import com.google.firebase.firestore.UserDataReader;
import com.google.firebase.firestore.auth.User;
import com.google.firebase.firestore.bundle.BundleReader;
import com.google.firebase.firestore.bundle.BundleSerializer;
Expand All @@ -57,6 +59,7 @@
import com.google.firebase.firestore.core.QueryListener;
import com.google.firebase.firestore.core.QueryOrPipeline;
import com.google.firebase.firestore.core.SyncEngine;
import com.google.firebase.firestore.core.Target;
import com.google.firebase.firestore.core.TargetOrPipeline;
import com.google.firebase.firestore.local.LocalStore;
import com.google.firebase.firestore.local.LruDelegate;
Expand Down Expand Up @@ -104,6 +107,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -163,6 +167,8 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
// separated by a space character.
private static final String TEST_FILTER_PROPERTY = "specTestFilter";

private static final String NO_PIPELINE_CONVERSION_TAG = "no-pipeline-conversion";

// 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 All @@ -174,6 +180,9 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
private boolean useEagerGcForMemory;
private int maxConcurrentLimboResolutions;
private boolean networkEnabled = true;
protected boolean usePipelineMode = false;

private FirebaseFirestore db;

//
// Parts of the Firestore system that the spec tests need to control.
Expand All @@ -196,7 +205,7 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
* A dictionary for tracking the listens on queries. Note that the identity of the listeners is
* used to remove them.
*/
private Map<Query, QueryListener> queryListeners;
private Map<QueryOrPipeline, QueryListener> queryListeners;

/**
* Set of documents that are expected to be in limbo with an active target. Verified at every
Expand Down Expand Up @@ -291,6 +300,7 @@ protected void specSetUp(JSONObject config) {

currentUser = User.UNAUTHENTICATED;
databaseInfo = PersistenceTestHelpers.nextDatabaseInfo();
db = new FirebaseFirestoreIntegrationTestFactory(databaseInfo.getDatabaseId()).firestore;

if (config.optInt("numClients", 1) != 1) {
throw Assert.fail("The Android client does not support multi-client tests");
Expand Down Expand Up @@ -577,21 +587,30 @@ private void doListen(JSONObject listenSpec) throws Exception {
Query query = parseQuery(listenSpec.getJSONObject("query"));
ListenOptions options = parseListenOptions(listenSpec);

QueryOrPipeline queryOrPipeline;
if (usePipelineMode) {
queryOrPipeline =
new QueryOrPipeline.PipelineWrapper(
query.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId())));
} else {
queryOrPipeline = new QueryOrPipeline.QueryWrapper(query);
}
Comment on lines +590 to +597
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic for creating a QueryOrPipeline from a Query is duplicated in doListen() and doUnlisten(). A similar pattern for creating RealtimePipeline instances exists in assertEventMatches(), validateExpectedSnapshotEvents(), and validateActiveTargets(). To improve maintainability and reduce code duplication, consider extracting this logic into a helper method.

For example:

private RealtimePipeline toRealtimePipeline(Query query) {
    return query.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId()));
}

private QueryOrPipeline createQueryOrPipeline(Query query) {
    if (usePipelineMode) {
        return new QueryOrPipeline.PipelineWrapper(toRealtimePipeline(query));
    } else {
        return new QueryOrPipeline.QueryWrapper(query);
    }
}


QueryListener listener =
new QueryListener(
new QueryOrPipeline.QueryWrapper(query),
queryOrPipeline,
options,
(value, error) -> {
QueryEvent event = new QueryEvent();
event.query = query;
event.queryOrPipeline = queryOrPipeline;
if (value != null) {
event.view = value;
} else {
event.error = error;
}
events.add(event);
});
queryListeners.put(query, listener);
queryListeners.put(queryOrPipeline, listener);
queue.runSync(
() -> {
int actualId = eventManager.addQueryListener(listener);
Expand All @@ -601,7 +620,15 @@ private void doListen(JSONObject listenSpec) throws Exception {

private void doUnlisten(JSONArray unlistenSpec) throws Exception {
Query query = parseQuery(unlistenSpec.get(1));
QueryListener listener = queryListeners.remove(query);
QueryOrPipeline queryOrPipeline;
if (usePipelineMode) {
queryOrPipeline =
new QueryOrPipeline.PipelineWrapper(
query.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId())));
} else {
queryOrPipeline = new QueryOrPipeline.QueryWrapper(query);
}
QueryListener listener = queryListeners.remove(queryOrPipeline);
queue.runSync(() -> eventManager.removeQueryListener(listener));
}

Expand Down Expand Up @@ -990,7 +1017,14 @@ private void doStep(JSONObject step) throws Exception {

private void assertEventMatches(JSONObject expected, QueryEvent actual) throws JSONException {
Query expectedQuery = parseQuery(expected.get("query"));
assertEquals(expectedQuery, actual.query);
if (usePipelineMode) {
assertEquals(
expectedQuery.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId())),
actual.queryOrPipeline.pipeline());
} else {
assertEquals(expectedQuery, actual.queryOrPipeline.query());
}

if (expected.has("errorCode") && !Status.fromCodeValue(expected.getInt("errorCode")).isOk()) {
assertNotNull(actual.error);
assertEquals(expected.getInt("errorCode"), actual.error.getCode().value());
Expand Down Expand Up @@ -1041,7 +1075,7 @@ private void validateExpectedSnapshotEvents(@Nullable JSONArray expectedEventsJs
}

// Sort both the expected and actual events by the query's canonical ID.
events.sort((q1, q2) -> q1.query.getCanonicalId().compareTo(q2.query.getCanonicalId()));
events.sort(Comparator.comparing(q -> q.queryOrPipeline.canonicalId()));

List<JSONObject> expectedEvents = new ArrayList<>();
for (int i = 0; i < expectedEventsJson.length(); ++i) {
Expand All @@ -1052,6 +1086,16 @@ private void validateExpectedSnapshotEvents(@Nullable JSONArray expectedEventsJs
try {
Query leftQuery = parseQuery(left.get("query"));
Query rightQuery = parseQuery(right.get("query"));
if (usePipelineMode) {
return leftQuery
.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId()))
.canonicalId()
.compareTo(
rightQuery
.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId()))
.canonicalId());
}

return leftQuery.getCanonicalId().compareTo(rightQuery.getCanonicalId());
} catch (JSONException e) {
throw new RuntimeException("Failed to parse JSON during event sorting", e);
Expand Down Expand Up @@ -1270,9 +1314,25 @@ private void validateActiveTargets() {
// with the single assertEquals on the TargetData objects themselves if the sequenceNumber is
// ever made to be consistent.
// assertEquals(expectedTarget, actualTarget);

assertEquals(expectedTarget.getPurpose(), actualTarget.getPurpose());
assertEquals(expectedTarget.getTarget(), actualTarget.getTarget());
if (usePipelineMode && !expectedTarget.getPurpose().equals(QueryPurpose.LIMBO_RESOLUTION)) {
Target target = expectedTarget.getTarget().target();
assertEquals(
new TargetOrPipeline.PipelineWrapper(
new Query(
target.getPath(),
target.getCollectionGroup(),
target.getFilters(),
target.getOrderBy(),
target.getLimit(),
Query.LimitType.LIMIT_TO_FIRST,
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

When reconstructing a Query from a Target to validate active targets in pipeline mode, Query.LimitType.LIMIT_TO_FIRST is hardcoded. This could lead to incorrect behavior if any spec tests use limitToLast. While it seems no current tests are affected, this makes the test logic brittle. It might be worth investigating if the limitType can be preserved in the Target object or if this limitation should be documented.

target.getStartAt(),
target.getEndAt())
.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId()))),
actualTarget.getTarget());
} else {
assertEquals(expectedTarget.getTarget(), actualTarget.getTarget());
}
assertEquals(expectedTarget.getTargetId(), actualTarget.getTargetId());
assertEquals(expectedTarget.getSnapshotVersion(), actualTarget.getSnapshotVersion());
assertEquals(
Expand Down Expand Up @@ -1392,6 +1452,10 @@ public void testSpecTests() throws Exception {
JSONArray steps = testJSON.getJSONArray("steps");
Set<String> tags = getTestTags(testJSON);

if (name.contains("Newer ")) {
info("Skipping test: " + name);
}
Comment on lines +1455 to +1457
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This check to skip tests containing "Newer " in their name seems like a temporary debugging tool. For better maintainability, it would be preferable to use a test tag, similar to NO_PIPELINE_CONVERSION_TAG, to control test execution.


boolean runTest;
if (!shouldRunTest(tags)) {
runTest = false;
Expand Down Expand Up @@ -1443,6 +1507,9 @@ private static boolean anyTestsAreMarkedExclusive(JSONObject fileJSON) throws JS

/** Called before executing each test to see if it should be run. */
private boolean shouldRunTest(Set<String> tags) {
if (usePipelineMode && tags.contains(NO_PIPELINE_CONVERSION_TAG)) {
return false;
}
return shouldRun(tags);
}

Expand Down
3 changes: 3 additions & 0 deletions firebase-firestore/src/test/resources/json/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
These json files are generated from the web test sources.

TODO(mikelehen): Re-add instructions for generating these.
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It's good that you've added a TODO here. To ensure this doesn't get lost, you might want to create a ticket in your issue tracking system to track this task.

Loading
Loading