[realppl 8] realppl spec tests #14861
Conversation
Using Gemini Code AssistThe 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
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 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. |
Generated by 🚫 Danger |
wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from 8139cf4 to f164452 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from 38b72bd to c2bf5ec Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from bcf6c5f to 7865fe4 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from c2bf5ec to b5ad143 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch 2 times, most recently from 22651c5 to e0ccb49 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from 5a19c8f to 8b8d4a2 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch 2 times, most recently from 0a8821c to 9292af4 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from 8b8d4a2 to 70c3278 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from 9292af4 to 958cee7 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from 70c3278 to 27a4b92 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from 958cee7 to affb6e1 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from 27a4b92 to bf5a534 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from affb6e1 to cc0e1cd Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from bf5a534 to e059610 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from cc0e1cd to 531ba2a Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from e059610 to 8d1e709 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from 531ba2a to 82b77f2 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from 8d1e709 to 587b678 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from 587b678 to c6f2775 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from 82b77f2 to a2c7960 Compare wu-hui force-pushed the wuandy/RealPpl_8 branch from c6f2775 to 0e39951 Compare wu-hui force-pushed the wuandy/RealPpl_7 branch from a2c7960 to 68e64f0 Compare 
wu-hui force-pushed the wuandy/RealPpl_8 branch from 0e39951 to 042515a Compare 
| /gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| // 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()); |
There was a problem hiding this comment.
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.
| 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()); | ||
| }]; |
There was a problem hiding this comment.
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.
| NSArray *queriesJson = queryData[@"queries"]; | ||
| std::vector<TargetData> queries; | ||
| for (id queryJson in queriesJson) { | ||
| core::QueryOrPipeline qop; |
There was a problem hiding this comment.
The variable qop is declared but never used within its scope. It should be removed to avoid confusion and potential compiler warnings.
4 participants
No description provided.