Skip to content

Add test for ReviewerControlPreference.getPreferenceAssignedTo (#13283) - #21423

Merged
david-allison merged 1 commit into
ankidroid:mainfrom
GuTS805:test/reviewer-control-preference-13283
Aug 7, 2026
Merged

Add test for ReviewerControlPreference.getPreferenceAssignedTo (#13283)#21423
david-allison merged 1 commit into
ankidroid:mainfrom
GuTS805:test/reviewer-control-preference-13283

Conversation

@GuTS805

@GuTS805 GuTS805 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Covers side-specific binding lookup: a binding on QUESTION/ANSWER only matches a preference on
the same side, while a binding on BOTH conflicts with any single-side binding. Verified via
mutation testing that re-introducing the bug (ignoring the card side) breaks the test.

Fixes part of #13283

Purpose / Description

ReviewerControlPreference.getPreferenceAssignedTo had no test coverage (@NeedsTest("Ensure correct preference is returned for side-specific binding")). This is one of many @NeedsTest
items tracked in #13283 — only this single method is covered here, per the maintainer's stated
preference elsewhere in that thread to keep PRs to one test at a time.

Approach

Added ReviewerControlPreferenceTest covering three cases: a binding on the same side is found,
a binding on a different side is not found, and a binding on CardSide.BOTH conflicts with a
single-side binding. Since getRelatedPreferences() matches preferences by exact class
(it::class == ReviewerControlPreference::class), the test uses real ReviewerControlPreference
instances (not a subclass) and accesses the protected side field and getPreferenceAssignedTo
method via reflection, following the existing pattern in NumberRangePreferenceCompatTest.

How Has This Been Tested?

Ran the new test in isolation and confirmed it fails when the side-overlap logic is broken
(temporarily changed ReviewerBinding(binding, cardSide) to always use CardSide.BOTH, confirmed
the "different side is not found" test failed, then reverted). Also ran the full
com.ichi2.preferences.* test package with no regressions.

Learning (optional, can help others)

ReviewerBinding.equals() treats CardSide.BOTH as conflicting with any other side, and two
same-typed sides as conflicting with each other — that's the "side overlap" logic this method
relies on via getPreferencesAssignedTo.

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

@welcome

welcome Bot commented Aug 7, 2026

Copy link
Copy Markdown

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@david-allison david-allison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't look self-reviewed: we've moved to SPDX copyright headers, and if you wish to add your name/email, it should be yours, not mine :D

Comment thread AnkiDroid/src/test/java/com/ichi2/preferences/ReviewerControlPreferenceTest.kt Outdated
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Aug 7, 2026
@GuTS805
GuTS805 force-pushed the test/reviewer-control-preference-13283 branch from 88e0a27 to 8518613 Compare August 7, 2026 18:27
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Aug 7, 2026
@david-allison
david-allison self-requested a review August 7, 2026 18:29

@david-allison david-allison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! One nitpick to be sure we're testing the behaviour we depend on

Comment thread AnkiDroid/src/test/java/com/ichi2/preferences/ReviewerControlPreferenceTest.kt Outdated
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Aug 7, 2026
Covers side-specific binding lookup: a binding on QUESTION/ANSWER only
matches a preference on the same side, while a binding on BOTH conflicts
with any single-side binding. Verified via mutation testing that
re-introducing the bug (ignoring the card side) breaks the test.

Fixes part of ankidroid#13283
@GuTS805
GuTS805 force-pushed the test/reviewer-control-preference-13283 branch from 8518613 to 7972528 Compare August 7, 2026 21:06

@david-allison david-allison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@GuTS805

GuTS805 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review.

@david-allison david-allison removed Needs Author Reply Waiting for a reply from the original author Needs Review labels Aug 7, 2026
@david-allison
david-allison enabled auto-merge August 7, 2026 21:14
@david-allison david-allison added the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Aug 7, 2026
@david-allison
david-allison added this pull request to the merge queue Aug 7, 2026
Merged via the queue into ankidroid:main with commit a166312 Aug 7, 2026
20 checks passed
@github-actions github-actions Bot added this to the 2.25 release milestone Aug 7, 2026
@github-actions github-actions Bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Aug 7, 2026
@GuTS805
GuTS805 deleted the test/reviewer-control-preference-13283 branch August 7, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants