Skip to content
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

test: android test for MultimediaFragment intents #17286

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

criticalAY
Copy link
Contributor

Purpose / Description

Test for multimedia activity intents i.e. camera, gallery & drawing

Approach

Use libs.androidx.espresso.intents to check if the intents are there or not, this will be useful to test other components too I.e cropper, audio recorder, etc

How Has This Been Tested?

Tested locally using ./gradlew connectedplayDebugAndroidTest

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Check the android test failures they seem related to your changes.

gradle/libs.versions.toml Outdated Show resolved Hide resolved
@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Oct 20, 2024
@criticalAY criticalAY force-pushed the test-multimedia-intent branch 2 times, most recently from 9546eda to bb31a0d Compare October 20, 2024 18:18
@criticalAY criticalAY force-pushed the test-multimedia-intent branch from bb31a0d to 094b1cc Compare October 20, 2024 19:44
@criticalAY criticalAY force-pushed the test-multimedia-intent branch from 094b1cc to 1e855ae Compare October 20, 2024 19:46
@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Oct 20, 2024
@lukstbit lukstbit added Needs Author Reply Waiting for a reply from the original author Needs reviewer reply Waiting for a reply from another reviewer labels Oct 21, 2024
@criticalAY criticalAY force-pushed the test-multimedia-intent branch from 1e855ae to cf2ad33 Compare October 21, 2024 14:43

This comment was marked as outdated.

@github-actions github-actions bot added the Stale label Nov 4, 2024
@criticalAY criticalAY removed the Stale label Nov 8, 2024

This comment was marked as outdated.

This comment was marked as outdated.

@github-actions github-actions bot added the Stale label Dec 7, 2024
@david-allison
Copy link
Member

david-allison commented Dec 8, 2024

@criticalAY with apologies for the delay on this one, is Needs Author Reply Waiting for a reply from the original author still valid, or is this open for review?

@criticalAY
Copy link
Contributor Author

Up for review

@criticalAY criticalAY removed the Stale label Dec 8, 2024
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Dec 8, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Take it or leave it. I'm happy either way, and CI is happy. I'm more than happy with more tests 😁

Let me know if you want an explanation on anything here.

Index: AnkiDroid/src/androidTest/java/com/ichi2/anki/MultimediaTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/androidTest/java/com/ichi2/anki/MultimediaTest.kt b/AnkiDroid/src/androidTest/java/com/ichi2/anki/MultimediaTest.kt
--- a/AnkiDroid/src/androidTest/java/com/ichi2/anki/MultimediaTest.kt	(revision cf2ad33710b66061e3a276d57334cdaf3afeb135)
+++ b/AnkiDroid/src/androidTest/java/com/ichi2/anki/MultimediaTest.kt	(date 1733658217414)
@@ -19,6 +19,7 @@
 
 import android.content.Context
 import android.content.Intent
+import androidx.fragment.app.Fragment
 import androidx.test.core.app.ActivityScenario
 import androidx.test.espresso.Espresso.onView
 import androidx.test.espresso.assertion.ViewAssertions.matches
@@ -38,7 +39,6 @@
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.Parameterized
-import kotlin.test.DefaultAsserter.assertEquals
 import kotlin.test.assertEquals
 
 @RunWith(Parameterized::class)
@@ -50,15 +50,22 @@
 
     @JvmField
     @Parameterized.Parameter(1)
-    var title: Int? = null
+    var expectedTitleRes: Int? = null
+
+    @Suppress("unused") // used by "{2}"
+    @JvmField
+    @Parameterized.Parameter(2)
+    var testName: String = ""
+
+    private val expectedTitle
+        get() = testContext.getString(expectedTitleRes!!)
 
     @Test
     fun testFragmentTitle() {
-        ActivityScenario.launch<MultimediaActivity>(intentBuilder(testContext)).use { scenario ->
+        withMultimediaActivityScenario { scenario ->
             scenario.onActivity { activity ->
-                val fragment = activity.supportFragmentManager.findFragmentById(R.id.fragment_container) as MultimediaFragment
-                val titleString = title?.let { testContext.getString(it) }
-                assertEquals(titleString, fragment.title)
+                val actualFragmentTitle = (activity.fragmentContainer as MultimediaFragment).title
+                assertEquals(expectedTitle, actualFragmentTitle, message = "title")
             }
         }
     }
@@ -66,7 +73,7 @@
     @Test
     fun testImageFragmentDiscardDialogShown() {
         val validIntentBuilders = setOf(
-            { context: Context -> getImageFragment(context) },
+            { context: Context -> getCameraFragment(context) },
             { context: Context -> getGalleryFragment(context) },
             { context: Context -> getDrawingFragment(context) }
         )
@@ -75,11 +82,12 @@
             return
         }
 
-        ActivityScenario.launch<MultimediaActivity>(intentBuilder(testContext)).use { scenario ->
+        withMultimediaActivityScenario { scenario ->
             scenario.onActivity { activity ->
-                val fragment = activity.supportFragmentManager.findFragmentById(R.id.fragment_container) as MultimediaImageFragment
-                fragment.viewModel.updateCurrentMultimediaPath("test/path")
-                fragment.requireActivity().onBackPressedDispatcher.onBackPressed()
+                (activity.fragmentContainer as MultimediaImageFragment).apply {
+                    viewModel.updateCurrentMultimediaPath("test/path")
+                    requireActivity().onBackPressedDispatcher.onBackPressed()
+                }
             }
 
             onView(withText(CollectionManager.TR.addingDiscardCurrentInput()))
@@ -94,23 +102,32 @@
         }
     }
 
+    /** Runs [ActivityScenario.launch] with the result of the [intentBuilder] */
+    private fun withMultimediaActivityScenario(block: (ActivityScenario<MultimediaActivity>) -> Unit) {
+        ActivityScenario.launch<MultimediaActivity>(intentBuilder(testContext)).use { block(it) }
+    }
+
+    private val MultimediaActivity.fragmentContainer: Fragment
+        get() = this.supportFragmentManager.findFragmentById(R.id.fragment_container)!!
+
+
     companion object {
-        @Parameterized.Parameters(name = "{index}: {1}")
+        @Parameterized.Parameters(name = "{2}")
         @JvmStatic
         fun initParameters(): Collection<Array<out Any>> {
             return listOf(
-                arrayOf({ context: Context -> getImageFragment(context) }, R.string.multimedia_editor_popup_image),
-                arrayOf({ context: Context -> getGalleryFragment(context) }, R.string.multimedia_editor_popup_image),
-                arrayOf({ context: Context -> getDrawingFragment(context) }, R.string.multimedia_editor_popup_image),
-                arrayOf({ context: Context -> getAudioFragment(context) }, R.string.multimedia_editor_popup_audio_clip),
-                arrayOf({ context: Context -> getVideoFragment(context) }, R.string.multimedia_editor_popup_video_clip),
-                arrayOf({ context: Context -> getAudioRecordingFragment(context) }, R.string.multimedia_editor_field_editing_audio)
+                arrayOf({ context: Context -> getCameraFragment(context) }, R.string.multimedia_editor_popup_image, "Add image (Camera)"),
+                arrayOf({ context: Context -> getGalleryFragment(context) }, R.string.multimedia_editor_popup_image, "Add image (Gallery)"),
+                arrayOf({ context: Context -> getDrawingFragment(context) }, R.string.multimedia_editor_popup_image, "Add image (Drawing)"),
+                arrayOf({ context: Context -> getAudioFragment(context) }, R.string.multimedia_editor_popup_audio_clip, "Add audio clip"),
+                arrayOf({ context: Context -> getVideoFragment(context) }, R.string.multimedia_editor_popup_video_clip, "Add video clip"),
+                arrayOf({ context: Context -> getAudioRecordingFragment(context) }, R.string.multimedia_editor_field_editing_audio, "Record audio")
             )
         }
 
         private val multimediaActivityExtra = MultimediaActivityExtra(0, ImageField(), getTestMultimediaNote())
 
-        private fun getImageFragment(context: Context): Intent {
+        private fun getCameraFragment(context: Context): Intent {
             return MultimediaImageFragment.getIntent(
                 context,
                 multimediaActivityExtra,

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs reviewer reply Waiting for a reply from another reviewer labels Dec 8, 2024
@lukstbit lukstbit closed this Dec 9, 2024
@lukstbit lukstbit reopened this Dec 9, 2024
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Ok, I didn't had any issues with the code itself just with how useful I perceived it to be.

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Dec 9, 2024
@criticalAY criticalAY force-pushed the test-multimedia-intent branch from cf2ad33 to f13818d Compare December 9, 2024 13:38
@criticalAY
Copy link
Contributor Author

@david-allison thanks! applied the patch

@david-allison david-allison added this pull request to the merge queue Dec 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 9, 2024
@mikehardy
Copy link
Member


com.ichi2.anki.MultimediaTest > testFragmentTitle[Add image (Camera)][emulator-5554 - 11] FAILED 
	java.lang.NullPointerException: Cannot run onActivity since Activity has been destroyed already
	at androidx.test.internal.util.Checks.checkNotNull(Checks.java:50)

That's not a good sign - adding a flaky test is worse than adding no test, this needs real investigation + real fix so it isn't flaky

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

flaky

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge and removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Dec 9, 2024
@criticalAY
Copy link
Contributor Author

I am reverting the patch and will run 20 iterations lets see then

@david-allison
Copy link
Member

@criticalAY to confirm: we have a GitHub action you can run for the 20x testing

@criticalAY
Copy link
Contributor Author

It is not poping up for emulated test but unit

@mikehardy
Copy link
Member

The Flake Hammer (tm) was implemented unit test only
mikehardy@e524c8f

@criticalAY
Copy link
Contributor Author

I have run emulated tests 16 times by manually triggering the CI, all green there

Copy link
Contributor

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants