-
Notifications
You must be signed in to change notification settings - Fork 114
Adds integration test to travel_app, standardizes AiModel #97
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.
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
Conversation
Adds comprehensive unit tests for the travel_app package. - Creates new tests for previously untested widgets: - `options_filter_chip.dart` - `section_header.dart` - Enhances existing tests for better coverage and assertions: - `itinerary_item_test.dart` - `trailhead_test.dart` - `travel_carousel_test.dart` - `asset_images_test.dart` - Cleans up unused test files and dependencies.
|
/gemini summary |
There was a problem hiding this 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 significantly improves the codebase by adding a comprehensive suite of tests, including a new integration test for the travel_app. The refactoring to standardize the AiModel and AiClient interface is a great step towards a more flexible and testable architecture. My review includes a few suggestions to improve type safety and code consistency. The PR description is minimal; consider expanding it to follow the repository's contribution guidelines, as mentioned in the style guide.
| import 'package:flutter_genui/flutter_genui.dart'; | ||
| import 'package:flutter_genui/src/ai_client/tools.dart'; | ||
|
|
||
| class FakeAiClient implements AiClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this go to test/test_infra too?
Or, to avoid duplicates we can export it from pkgs/flutter_genui/lib/testing.dart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm. Yeah, I thought about that, but decided to duplicate for now. It felt weird exporting a fake in the public interface because they often are specific to the tests you're writing.
I can move it, though.
polina-c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, then LGTM
I've been working on standardizing the AI model abstraction within
flutter_genuito improve flexibility and testability. This pull request introduces a newAiModelinterface and refactors theGeminiAiClientto conform to it. As a direct benefit, I've added a new integration test for thetravel_appexample, which utilizes a mock AI client made possible by this standardization. Additionally, I've included several new unit tests for various UI components in thetravel_appand made a minor update to thegeneric_chatexample.Highlights
AiModelabstract class and updated theAiClientinterface to use this abstraction. This provides a standardized way to represent and interact with different AI models, making the system more flexible and extensible.GeminiAiClienthas been refactored to fully integrate with the newAiModelinterface. This involved renaming theGeminiModelenum toGeminiModelTypeand creating aGeminiModelclass that extendsAiModel, ensuring compatibility and proper model management.travel_appexample. This test simulates user interactions and verifies the application's UI behavior when responding to AI-generated content, leveraging aMockAiClientmade possible by theAiModelstandardization.MyApptoTravelAppand the main page widget fromMyHomePagetoTravelPlannerPagein thetravel_appexample. Their constructors now also support dependency injection ofAiClientinstances.There are still some places where we depend on
firebase_aithat we shouldn't (in ai_client.dart, for instance), because of the lack of a genericSchema,Content, andPartsubstitute.Fixes #39
Addresses part of #60