Skip to content

Fix test: Add unit test for collapsed decks in disableDeckAndChildrenShortcuts#20364

Open
DoomsCoder wants to merge 1 commit intoankidroid:mainfrom
DoomsCoder:fix/collapsed-deck-shortcuts-test
Open

Fix test: Add unit test for collapsed decks in disableDeckAndChildrenShortcuts#20364
DoomsCoder wants to merge 1 commit intoankidroid:mainfrom
DoomsCoder:fix/collapsed-deck-shortcuts-test

Conversation

@DoomsCoder
Copy link
Contributor

@DoomsCoder DoomsCoder commented Feb 25, 2026

Purpose / Description

Adds a missing regression unit test for the disableDeckAndChildrenShortcuts flow in DeckPickerViewModel.

When a parent deck is collapsed, disabling shortcuts should also include all of its child decks. This test ensures that the emitted shortcut disable list correctly contains the parent deck and all descendant deck Ids.
This prevent a future regression where hidden child decks could be missed during this process.

Fixes

Approach

  • Created a synthetic deck tree using DeckTreeNode and DeckNode.
  • Configured a collapsed parent deck with two child decks.
  • Mocked Collection and Scheduler to return the prepared deck tree
  • Triggered reloadDeckCounts() to populate dueTree
  • Invoked disableDeckAndChildrenShortcuts(deckId)

The test verifies that:

  • The emitted Id list includes:
    1.Parent deck Id
    2. All child deck Ids
  • The size and contents match the expected hierarchy.
    A custom MainCoroutinesRule was added to control coroutine execution and ensure deterministic test behavior.

How Has This Been Tested?

Test Type:
Unit test(/test, not /androidTest)

Steps to Reproduce:

  1. Mock Collection and Scheduler
  2. Return a prepared deck tree via deckDueTree()
  3. Call:
    viewmodel.reloadDeckCounts().join()
    viemodel.disableDeckAndChildrenShortcuts(parentId)
    4.Collect from:
    flowOfDisableShortcuts
    5.Assert emitted IDs match expected deck hierarchy.

Learning (optional, can help others)

Key learnings while implementing this test:

  • DeckNode.children are derived from DeckTreeNode.childrenList, not manually assignable
  • Synthetic deck nodes (level <= 0) are excluded from iteration
  • reloadDeckCounts().join() is required to ensure dueTree is populated before assertions
  • Coroutine dispatchers must be overridden in tests for detereministic execution

Checklist

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

Thank you for contributing.
The proposed code works but it's way to complicated for what is trying to achieve.
I left a patch below with a much simpler implementation to use:

diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt
index ae5f0ee83b..24bd2bfca6 100644
--- a/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt
@@ -35,6 +35,7 @@ import org.hamcrest.Matchers.equalTo
 import org.junit.Test
 import org.junit.runner.RunWith
 import timber.log.Timber
+import kotlin.test.assertEquals
 
 /** Test of [DeckPickerViewModel] */
 @RunWith(AndroidJUnit4::class)
@@ -147,6 +148,29 @@ class DeckPickerViewModelTest : RobolectricTest() {
         }
     }
 
+    @Test
+    fun `ensure collapsed decks are also deleted when disabling shortcuts`() =
+        runTest {
+            val deckIdA = addDeck("A")
+            val subDeckIdA1 = addDeck("A::A1")
+            val subDeckIdA2 = addDeck("A::A2")
+            // add other decks as well as control
+            addDeck("B")
+            addDeck("B:B1")
+            viewModel.flowOfDisableShortcuts.test {
+                viewModel.reloadDeckCounts().join()
+                viewModel.disableDeckAndChildrenShortcuts(deckIdA)
+                val actual = awaitItem()
+                assertEquals(
+                    listOf(
+                        deckIdA.toString(),
+                        subDeckIdA1.toString(),
+                        subDeckIdA2.toString(),
+                    ), actual
+                )
+            }
+        }
+
     /**
      * Creates a note with 3 cards, all empty
      *

Also consider squashing into a single commit.

@lukstbit lukstbit added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Feb 28, 2026
@DoomsCoder
Copy link
Contributor Author

Hi @lukstbit
Thank you so much for the detailed review and for providing the simpler implementation! This is incredibly helpful.
I was definitely going down a more complicated path with mocks to isolate the viewmodel. I see now that using the test's real collection with addDeck is much cleaner and more direct for this scenario.

I will update my PR with your suggested approach and also squash commits. Thanks again for the guidance!

@DoomsCoder DoomsCoder force-pushed the fix/collapsed-deck-shortcuts-test branch from 189b422 to ba13849 Compare February 28, 2026 20:53
@DoomsCoder DoomsCoder requested a review from lukstbit February 28, 2026 21:21
@DoomsCoder
Copy link
Contributor Author

Thanks again for the guidance. I have updated the test based on your suggestion and squashed the commits into single commit. The PR should now be ready for review.

…ortcuts

Simplified test implementation based on reviewer suggestion.
@DoomsCoder DoomsCoder force-pushed the fix/collapsed-deck-shortcuts-test branch from ba2f90c to 65a826a Compare February 28, 2026 22:14
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants