Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: Android CI

on:
push:
branches: [ main ]
pull_request:
branches: [ main ]

jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Check out code
uses: actions/checkout@v3

- name: Set up JDK 17
uses: actions/setup-java@v3
with:
java-version: '17'
distribution: 'temurin'

- name: Set up Android SDK
uses: android-actions/setup-android@v3 # Using v3 as it's the latest
with:
api-level: 34 # Assuming API level 34, adjust if needed
build-tools-version: 34.0.0 # Assuming build tools 34.0.0
ndk-version: 25.1.8937393 # Optional: specify NDK version if needed
# cmake-version: # Optional: specify CMake version if needed

- name: Grant execute permission to gradlew
run: chmod +x ai-catalog/gradlew

- name: Build with Gradle
working-directory: ./ai-catalog
run: ./gradlew build

- name: Run Android Instrumented Tests
working-directory: ./ai-catalog
run: ./gradlew :app:connectedAndroidTest
4 changes: 4 additions & 0 deletions ai-catalog/app/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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?

androidTestImplementation("androidx.test.ext:junit:1.1.5")
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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")
Comment on lines +98 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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

  1. 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.

androidTestImplementation(platform(libs.androidx.compose.bom))
androidTestImplementation(libs.androidx.ui.test.junit4)
debugImplementation(libs.androidx.ui.tooling)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.google.android.apps.ai.catalog;

import androidx.test.ext.junit.runners.AndroidJUnit4;
import org.junit.runner.RunWith;
import org.junit.Rule;
import androidx.test.rule.ActivityTestRule;
import com.google.android.apps.ai.catalog.MainActivity;
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

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?

Suggested change
import com.google.android.apps.ai.catalog.MainActivity;
import com.android.ai.catalog.MainActivity;


import static androidx.test.espresso.Espresso.onView;
import static androidx.test.espresso.assertion.ViewAssertions.matches;
import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed;
import static androidx.test.espresso.matcher.ViewMatchers.withText;
import org.junit.Test;

@RunWith(AndroidJUnit4.class)
public class MainActivityTest {

@Rule
public ActivityTestRule<MainActivity> activityRule =
new ActivityTestRule<>(MainActivity.class);
Comment on lines +19 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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

Suggested change
public ActivityTestRule<MainActivity> activityRule =
new ActivityTestRule<>(MainActivity.class);
public ActivityScenarioRule<MainActivity> activityScenarioRule =
new ActivityScenarioRule<>(MainActivity.class);

Footnotes

  1. Android testing best practices recommend migrating from the deprecated ActivityTestRule to ActivityScenarioRule for improved test reliability and lifecycle management. ActivityScenarioRule is 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()));
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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?

  1. Add this string to res/values/strings.xml (e.g., <string name="main_activity_title">Android AI Samples</string>).
  2. 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

  1. 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.

}
}
Loading