[realppl 4] Port spec tests for realtime ppl #7352
Conversation
Firebase AI Mock Responses Check
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @wu-hui, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on porting existing spec tests to support a new 'realtime pipeline' mode. It involves adapting the test infrastructure to allow tests to run against both traditional query implementations and the new pipeline, ensuring compatibility and correctness for future features. The changes primarily affect test-related code and data structures, laying the groundwork for comprehensive testing of the pipeline.
Highlights
- New Pipeline Test Modes: Introduced
MemoryPipelineSpecTest.javaandSQLitePipelineSpecTest.javato enable testing against a new 'realtime pipeline' mode. These new test classes extend existing spec tests and activate the pipeline mode. - QueryEvent and SpecTestCase Adaptations: Modified
QueryEvent.javato supportQueryOrPipelineobjects, allowing it to handle both traditional queries and the new pipeline representations.SpecTestCase.javawas updated to conditionally use the pipeline mode for query listeners, unlisteners, and event assertions, based on ausePipelineModeflag. - Updated Spec Test Data: Numerous JSON spec test files (e.g.,
collection_spec_test.json,garbage_collection_spec_test.json,orderby_spec_test.json) were updated to includepipelinesobjects within their expected state and event structures, reflecting the data representation for the new pipeline mode. - Test Skipping Logic: Added logic to
SpecTestCase.javato skip tests tagged withno-pipeline-conversionwhen running in pipeline mode, and temporarily skip tests with 'Newer ' in their name.
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Generated by 🚫 Danger |
There was a problem hiding this comment.
Code Review
This pull request ports the spec tests to support a new "pipeline mode" for realtime queries. The changes introduce a usePipelineMode flag in SpecTestCase and conditionally create and validate QueryOrPipeline objects. The test data files are also updated with expected pipeline definitions. The changes are logical and well-contained. I have a few suggestions to improve code maintainability by reducing duplication and making test configuration more robust.
| QueryOrPipeline queryOrPipeline; | ||
| if (usePipelineMode) { | ||
| queryOrPipeline = | ||
| new QueryOrPipeline.PipelineWrapper( | ||
| query.toRealtimePipeline(db, new UserDataReader(databaseInfo.getDatabaseId()))); | ||
| } else { | ||
| queryOrPipeline = new QueryOrPipeline.QueryWrapper(query); | ||
| } |
There was a problem hiding this comment.
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);
}
}| target.getFilters(), | ||
| target.getOrderBy(), | ||
| target.getLimit(), | ||
| Query.LimitType.LIMIT_TO_FIRST, |
There was a problem hiding this comment.
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.
| if (name.contains("Newer ")) { | ||
| info("Skipping test: " + name); | ||
| } |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,3 @@ | |||
| These json files are generated from the web test sources. | |||
|
|
|||
| TODO(mikelehen): Re-add instructions for generating these. | |||
There was a problem hiding this comment.
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.
|
The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Size Report 1Affected Products
Test Logs |
wu-hui
force-pushed
the
wuandy/RealPpl_4
branch
from
3187cd1 to
748f6cf
Compare
wu-hui
force-pushed
the
wuandy/RealPpl_3
branch
from
bfc920f to
34bbc4a
Compare
|
The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
1 similar comment
|
The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
wu-hui
force-pushed
the
wuandy/RealPpl_4
branch
from
748f6cf to
270725f
Compare
|
The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
2 participants
No description provided.