[Jules] Add instrumented test for catalog app to verify app launches #39
Conversation
This commit adds an instrumented test that verifies: 1. The MainActivity of the ai-catalog app launches successfully. 2. A TextView with the text "Android AI Samples" is displayed on the screen. The test is located in `ai-catalog/app/src/androidTest/java/com/google/android/apps/ai/catalog/MainActivityTest.java`. Dependencies for Espresso and AndroidX Test libraries have been added to `ai-catalog/app/build.gradle.kts`. Note: I skipped test execution due to an environment configuration issue (Android SDK not found). The test is expected to pass in an environment with a correctly configured Android SDK.
There was a problem hiding this comment.
Hello @calren, 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!
Summary of Changes
This pull request adds an instrumented test for the catalog app to verify that the app launches successfully. It includes adding necessary dependencies for Android UI testing and creating a MainActivityTest class with a test case that checks if the main activity launches and displays the text "Android AI Samples".
Highlights
- Instrumentation Test: Adds an instrumentation test to verify the catalog app launches successfully.
- Dependencies: Adds necessary dependencies for Android UI testing, including Espresso, JUnit extensions, and test runner/rules.
- UI Assertion: The test asserts that the text "Android AI Samples" is displayed on the screen upon app launch.
Changelog
- ai-catalog/app/build.gradle.kts
- Added dependencies for Android UI testing: espresso-core, androidx.test.ext:junit, androidx.test:runner, and androidx.test:rules.
- ai-catalog/app/src/androidTest/java/com/google/android/apps/ai/catalog/MainActivityTest.java
- Created a new
MainActivityTestclass. - Added a test case
testLaunchAndFindTextto verify the app launches and displays the expected text.
- Created a new
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.
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.
A test to run,
To see the app has begun,
Espresso's call,
Ensures all,
Displays as it should, everyone.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces an instrumented test for the catalog app, aiming to verify that the application launches successfully and displays an expected text element. This is a valuable addition for ensuring basic app functionality.
My review has identified a few areas for improvement, including a critical package import issue in the test, some dependency management concerns in the build.gradle.kts file, and suggestions for adhering to Android testing best practices. Since no specific style guide was provided for this repository, my feedback and suggestions are based on common Android development guidelines, Gradle best practices, and Android testing recommendations.
Addressing these points will enhance the robustness and maintainability of the test and the project's dependency setup.
Summary of Findings
- Critical: Incorrect MainActivity Package Import: The test file
MainActivityTest.javaimportsMainActivityfrom an incorrect package (com.google.android.apps.ai.catalog.MainActivityinstead ofcom.android.ai.catalog.MainActivity), which will prevent compilation. - High: Redundant and Outdated Dependencies: The
build.gradle.ktsfile includes redundantespresso-coreandandroidx.test.ext:junitdependencies with older versions than those already managed by thelibs.versions.tomlcatalog. - Medium: Hardcoded Dependency Versions: New test dependencies (
androidx.test:runner,androidx.test:rules) inbuild.gradle.ktsuse hardcoded versions instead of being managed through thelibs.versions.tomlcatalog, which is a recommended practice for maintainability. - Medium: Use of Deprecated
ActivityTestRule:MainActivityTest.javauses the deprecatedActivityTestRule. It should be updated toActivityScenarioRuleas per current Android testing best practices. - Medium: Hardcoded String in UI Test Assertion: The test
testLaunchAndFindTextinMainActivityTest.javauses a hardcoded string ("Android AI Samples") for assertion. Using string resources (R.string.*) is recommended for better maintainability and resilience to UI changes. - Low: Import Order (Not Commented): The import statements in
MainActivityTest.javacould be organized according to standard Java/Android conventions for better readability. (Severity: low, not commented due to review settings) - Low: Missing Javadoc (Not Commented): The
MainActivityTestclass and its test methodtestLaunchAndFindTextlack Javadoc comments explaining their purpose. (Severity: low, not commented due to review settings)
Merge Readiness
This pull request introduces a useful basic UI test. However, there are several issues that should be addressed before merging, including a critical compilation error due to an incorrect package import, high-severity redundant dependencies, and medium-severity concerns regarding dependency management and adherence to current Android testing best practices.
I recommend that these changes, particularly the critical and high severity issues, be addressed. As a reviewer, I am not authorized to approve pull requests, but based on this review, changes are requested. Please ensure further review and approval by authorized maintainers after the suggested modifications are made.
| import org.junit.runner.RunWith; | ||
| import org.junit.Rule; | ||
| import androidx.test.rule.ActivityTestRule; | ||
| import com.google.android.apps.ai.catalog.MainActivity; |
There was a problem hiding this comment.
There seems to be a package mismatch for the MainActivity import. The MainActivity.kt file is located in the com.android.ai.catalog package (as per its package declaration and project structure), but this test file (in com.google.android.apps.ai.catalog) attempts to import MainActivity from com.google.android.apps.ai.catalog.MainActivity.
This will likely cause a compilation error. Shouldn't the import be for com.android.ai.catalog.MainActivity to correctly reference the activity under test?
| import com.google.android.apps.ai.catalog.MainActivity; | |
| import com.android.ai.catalog.MainActivity; |
| @@ -93,6 +93,10 @@ dependencies { | |||
| testImplementation(libs.junit) | |||
| androidTestImplementation(libs.androidx.junit) | |||
| androidTestImplementation(libs.androidx.espresso.core) | |||
| androidTestImplementation("androidx.test.espresso:espresso-core:3.5.1") | |||
There was a problem hiding this comment.
It appears this line introduces a redundant dependency for espresso-core. Line 95 already includes androidTestImplementation(libs.androidx.espresso.core), which, according to your libs.versions.toml (espressoCore = "3.6.1"), points to version 3.6.1.
This new line adds espresso-core:3.5.1, which is an older version and duplicates the dependency. Could this line be removed to avoid potential version conflicts and keep dependencies streamlined via the versions catalog?
| @@ -93,6 +93,10 @@ dependencies { | |||
| testImplementation(libs.junit) | |||
| androidTestImplementation(libs.androidx.junit) | |||
| androidTestImplementation(libs.androidx.espresso.core) | |||
| androidTestImplementation("androidx.test.espresso:espresso-core:3.5.1") | |||
| androidTestImplementation("androidx.test.ext:junit:1.1.5") | |||
There was a problem hiding this comment.
Similar to the espresso-core dependency, this line for androidx.test.ext:junit seems redundant. Line 94 includes androidTestImplementation(libs.androidx.junit), which your libs.versions.toml (junitVersion = "1.2.1") defines for androidx.test.ext:junit at version 1.2.1.
This new line adds androidx.test.ext:junit:1.1.5, an older version. Would it be appropriate to remove this line to rely on the version managed by the catalog and prevent redundancy?
| androidTestImplementation("androidx.test:runner:1.5.2") | ||
| androidTestImplementation("androidx.test:rules:1.5.0") |
There was a problem hiding this comment.
These test dependencies (androidx.test:runner and androidx.test:rules) are added with hardcoded versions. For better version management and consistency across the project, it's a good practice to define these versions in your gradle/libs.versions.toml file and reference them here using the libs catalog1.
For example, you could add entries to your libs.versions.toml like:
[versions]
# ... other versions
androidxTestRunner = "1.5.2"
androidxTestRules = "1.5.0"
[libraries]
# ... other libraries
androidx-test-runner = { group = "androidx.test", name = "runner", version.ref = "androidxTestRunner" }
androidx-test-rules = { group = "androidx.test", name = "rules", version.ref = "androidxTestRules" }And then reference them here as:
androidTestImplementation(libs.androidx.test.runner)
androidTestImplementation(libs.androidx.test.rules)What are your thoughts on moving these to the versions catalog for improved maintainability?
androidTestImplementation(libs.androidx.test.runner) // Ensure this is defined in libs.versions.toml
androidTestImplementation(libs.androidx.test.rules) // Ensure this is defined in libs.versions.toml
Style Guide References
Footnotes
-
Gradle best practices, particularly with version catalogs (
libs.versions.toml), encourage centralizing dependency versions. This improves maintainability, reduces the chance of version conflicts, and makes updates easier across the project. ↩
| public ActivityTestRule<MainActivity> activityRule = | ||
| new ActivityTestRule<>(MainActivity.class); |
There was a problem hiding this comment.
ActivityTestRule is deprecated. It's recommended to use ActivityScenarioRule from androidx.test.ext.junit.rules.ActivityScenarioRule for better control over the activity lifecycle and more consistent test behavior1.
Could you update this to use ActivityScenarioRule? This would also involve changing the import from androidx.test.rule.ActivityTestRule to androidx.test.ext.junit.rules.ActivityScenarioRule.
Style Guide References
| public ActivityTestRule<MainActivity> activityRule = | |
| new ActivityTestRule<>(MainActivity.class); | |
| public ActivityScenarioRule<MainActivity> activityScenarioRule = | |
| new ActivityScenarioRule<>(MainActivity.class); |
Footnotes
-
Android testing best practices recommend migrating from the deprecated
ActivityTestRuletoActivityScenarioRulefor improved test reliability and lifecycle management.ActivityScenarioRuleis part of AndroidX Test, provides a more flexible API, and is the standard for new instrumented tests. ↩
|
|
||
| @Test | ||
| public void testLaunchAndFindText() { | ||
| onView(withText("Android AI Samples")).check(matches(isDisplayed())); |
There was a problem hiding this comment.
The test assertion uses a hardcoded string "Android AI Samples". While this works, it can make the test brittle if the UI text changes or if the app is localized. It's generally a best practice to use string resources (R.string.*) for such text, making tests more maintainable and resilient to UI copy updates1.
Could you consider the following improvements?
- Add this string to
res/values/strings.xml(e.g.,<string name="main_activity_title">Android AI Samples</string>). - Retrieve it in the test using
InstrumentationRegistry.getInstrumentation().getTargetContext().getString(R.string.main_activity_title).
This would also require importing androidx.test.platform.app.InstrumentationRegistry and your app's R class (e.g., com.android.ai.catalog.R, considering the MainActivity's actual package).
String expectedText = androidx.test.platform.app.InstrumentationRegistry.getInstrumentation().getTargetContext().getString(com.android.ai.catalog.R.string.main_activity_title); // Ensure 'main_activity_title' is defined in strings.xml and R class is correctly referenced.
onView(withText(expectedText)).check(matches(isDisplayed()));Style Guide References
Footnotes
-
In Android testing, using string resources (e.g.,
R.string.your_string_id) instead of hardcoded strings for UI text verification enhances test maintainability. It decouples tests from specific string literals, making them more robust against UI text changes and future localization efforts. ↩
This commit introduces a new GitHub workflow file `.github/workflows/ci.yml`. This workflow will: - Trigger on push and pull_request events to the `main` branch. - Check out the repository. - Set up JDK 17. - Set up the Android SDK (API 34, build-tools 34.0.0). - Grant execution permissions to the `ai-catalog/gradlew` wrapper. - Build the `ai-catalog` application using `./gradlew build`. - Run Android instrumented tests for the `ai-catalog:app` module using `./gradlew :app:connectedAndroidTest`. This will help ensure that the application builds successfully and all tests pass before merging changes into the main branch.
No description provided.