Skip to content

[realppl 8] realppl spec tests #14861

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

Jump to bottom
Open
wu-hui wants to merge 1 commit into
base: wuandy/RealPpl_7
Choose a base branch
from
Open

[realppl 8] realppl spec tests #14861

wu-hui wants to merge 1 commit into from

Conversation

@wu-hui
Copy link
Contributor

@wu-hui wu-hui commented May 16, 2025

No description provided.

@gemini-code-assist
Copy link
Contributor

gemini-code-assist bot commented May 16, 2025

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

@google-oss-bot
Copy link

google-oss-bot commented May 16, 2025

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch from 4dd578c to 6b3e105 Compare May 27, 2025 06:49
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch from 181bf74 to 6d02d69 Compare May 27, 2025 06:49
@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch from 6b3e105 to 76e5b20 Compare June 3, 2025 05:43
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch from 6d02d69 to 0135d82 Compare June 3, 2025 05:43
@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch from 76e5b20 to b95b616 Compare June 30, 2025 18:37
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from 8139cf4 to f164452 Compare July 3, 2025 17:38
@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch from b95b616 to 8dd3591 Compare July 3, 2025 17:38
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch from f164452 to 38b72bd Compare July 7, 2025 17:19
@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch from 8dd3591 to bcf6c5f Compare July 7, 2025 17:19
@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch 2 times, most recently from 22651c5 to e0ccb49 Compare September 12, 2025 18:25
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from 5a19c8f to 8b8d4a2 Compare September 12, 2025 18:30
@wu-hui wu-hui force-pushed the wuandy/RealPpl_7 branch 2 times, most recently from 0a8821c to 9292af4 Compare September 15, 2025 15:40
@wu-hui wu-hui requested a review from ehsannas September 17, 2025 15:08
@cherylEnkidu
Copy link
Contributor

cherylEnkidu commented Oct 15, 2025

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 15, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant changes to enable running Firestore spec tests in a new "pipeline mode". The core logic involves adding a usePipelineMode flag and updating test infrastructure, including FSTSpecTests and FSTSyncEngineTestDriver, to handle both traditional queries and new pipeline-based queries. This includes new validation, sorting, and comparison logic. The changes also enable pipeline support in EventManager and remote_event.cc. Numerous JSON spec tests have been added and updated to cover pipeline scenarios and fix existing issues. The overall approach is solid, but I've identified a potential compilation error, an unused variable, and an opportunity for refactoring to improve code clarity.

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
// Assuming self.driver.databaseInfo is accessible and provides it.
// This might require making databaseInfo public or providing a getter in
// FSTSyncEngineTestDriver. For now, proceeding with the assumption it's available.
auto serializer = absl::make_unique<remote::Serializer>(self.driver.databaseInfo.database_id());
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 15, 2025

Choose a reason for hiding this comment

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

high

The comment on the preceding lines correctly identifies that databaseInfo is needed, but the implementation self.driver.databaseInfo will not compile as databaseInfo is not a public property on FSTSyncEngineTestDriver. You'll need to expose this, perhaps by adding a read-only property to FSTSyncEngineTestDriver.h. This issue is also present in validateExpectedSnapshotEvents.

Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

I am confused, isn't databaseInfo already a public member?

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
Comment on lines +840 to +873
expectedEvents = [expectedEvents sortedArrayUsingComparator:^NSComparisonResult(
NSDictionary *left, NSDictionary *right) {
// Expected query from JSON is always a core::Query.
// For sorting consistency with actual events (which might be pipelines),
// we convert the expected query to QueryOrPipeline then get its CanonicalId.
// If _convertToPipeline is true, this will effectively sort expected items
// by their pipeline canonical ID.
Query leftJSONQuery = [self parseQuery:left[@"query"]];
core::QueryOrPipeline leftQoP;
if (self->_convertToPipeline) {
std::vector<std::shared_ptr<api::EvaluableStage>> stages =
core::ToPipelineStages(leftJSONQuery);
auto serializer =
absl::make_unique<remote::Serializer>(self.driver.databaseInfo.database_id());
leftQoP =
core::QueryOrPipeline(api::RealtimePipeline(std::move(stages), std::move(serializer)));
} else {
leftQoP = core::QueryOrPipeline(leftJSONQuery);
}

Query rightJSONQuery = [self parseQuery:right[@"query"]];
core::QueryOrPipeline rightQoP;
if (self->_convertToPipeline) {
std::vector<std::shared_ptr<api::EvaluableStage>> stages =
core::ToPipelineStages(rightJSONQuery);
auto serializer =
absl::make_unique<remote::Serializer>(self.driver.databaseInfo.database_id());
rightQoP =
core::QueryOrPipeline(api::RealtimePipeline(std::move(stages), std::move(serializer)));
} else {
rightQoP = core::QueryOrPipeline(rightJSONQuery);
}
return WrapCompare(leftQoP.CanonicalId(), rightQoP.CanonicalId());
}];
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 15, 2025

Choose a reason for hiding this comment

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

medium

The logic to convert a JSON query description into a QueryOrPipeline is duplicated for left and right inside this comparator. This could be extracted into a helper method to improve readability and reduce code duplication. For example, you could create a method like (core::QueryOrPipeline)queryOrPipelineFromQuerySpec:(NSDictionary *)querySpec.

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
NSArray *queriesJson = queryData[@"queries"];
std::vector<TargetData> queries;
for (id queryJson in queriesJson) {
core::QueryOrPipeline qop;
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 15, 2025

Choose a reason for hiding this comment

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

low

The variable qop is declared but never used within its scope. It should be removed to avoid confusion and potential compiler warnings.

Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

+ 1

cherylEnkidu
cherylEnkidu requested changes Nov 19, 2025
View reviewed changes
Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
// Assuming self.driver.databaseInfo is accessible and provides it.
// This might require making databaseInfo public or providing a getter in
// FSTSyncEngineTestDriver. For now, proceeding with the assumption it's available.
auto serializer = absl::make_unique<remote::Serializer>(self.driver.databaseInfo.database_id());
Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

I am confused, isn't databaseInfo already a public member?

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
NSArray *queriesJson = queryData[@"queries"];
std::vector<TargetData> queries;
for (id queryJson in queriesJson) {
core::QueryOrPipeline qop;
Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

+ 1

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
- (void)testSpecTests {
if ([self isTestBaseClass]) return;

// LogSetLevel(firebase::firestore::util::kLogLevelDebug);
Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

This should be removed

Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

also for the comments below

Firestore/Example/Tests/SpecTests/json/listen_source_spec_test.json
],
"errorCode": 0,
"fromCache": true,
"fromCache": false,
Copy link
Contributor

@cherylEnkidu cherylEnkidu Oct 16, 2025

Choose a reason for hiding this comment

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

May I ask why this one is set to false?

@cherylEnkidu cherylEnkidu assigned wu-hui and unassigned ehsannas Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cherylEnkidu cherylEnkidu cherylEnkidu requested changes

@ehsannas ehsannas Awaiting requested review from ehsannas

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wu-hui wu-hui

Labels

api: firestore

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wu-hui @google-oss-bot @cherylEnkidu @ehsannas