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

[Cleanup]: Add Tests to the code (@NeedsTest) #13283

Open
david-allison opened this issue Feb 16, 2023 · 10 comments
Open

[Cleanup]: Add Tests to the code (@NeedsTest) #13283

david-allison opened this issue Feb 16, 2023 · 10 comments
Labels
Good First Issue! Keep Open avoids the stale bot

Comments

@david-allison
Copy link
Member

david-allison commented Feb 16, 2023

Note

This issue does not need to be assigned to you before you work on it

A number of paths of the code have been identified as requiring automated testing.

Typically this is regression testing: if a bug has been found, then it's useful to have a unit test which would have failed due to the bug.

  1. Find a usage of @NeedsTest: https://github.com/ankidroid/Anki-Android/blob/main/AnkiDroid/src/main/java/com/ichi2/annotations/NeedsTest.kt
  2. Post the test(s) that you're handling on this issue, so people don't duplicate work
  3. Write a unit test (/test, not /androidTest)
  4. Ensure that re-introducing the bug or issue would break the test
  5. Submit a Pull Request
@Prithvi101
Copy link

please assign this issue to me

@david-allison
Copy link
Member Author

Multiple people can work on this issue, please pick a test and let us know!

@oakkitten
Copy link
Contributor

I suggest clarifying @NeedsTest comments. It is often hard to understand what exactly needs to be tested, and why. For example,

  • @NeedsTest("New User: Accepts permission") in StartupStoragePermissionManager. It is not clear what needs to be tested. A test tests behavior, that's when we do x and expect y. Here the comment doesn't specify y, and does a poor job of describing x. Where do we start? StartupStoragePermissionManager doesn't have a definitive entry point. Where do we finish? From reading the code I'm assuming that we expect onRegularStartup() to be called, but I am not sure.

  • @NeedsTest("Ensure a toast is shown on a successful action") in CreateDeckDialog. Here buth x and y are specified well enough, but I don't see the reason we need this test. We are not asking to test that creating a deck can succeed, but only that a toast is shown. Assuming there's code like if (success) showToast(), such a test would be testing Kotlin's if statement, which is, I'm assuming, was not what the annotation author wanted to test.

  • In a similar fashion, @NeedsTest("Selecting APKG allows multiple files") in ImportFileSelectionFragment would be testing Android APIs, not the app.

  • @NeedsTest("removing JvmOverloads should fail"). Removing JvmOverloads is not behavior of the app or user, but of the programmer. What would the test be like? Can we make a test that would somehow recompile the app, removing JvmOverloads, and then run it in an emulator and verify that it crashes? Suppose this test fails, how does this help us?

@Shubham-Patel07
Copy link

I am intrested to look into this issue ,Please assign this issue to me

@david-allison
Copy link
Member Author

This issue does not need to be assigned to you before you work on it

@tomaszgarbus
Copy link
Contributor

Question: are unit tests just preferred over UI tests or is it a hard requirement?

It seems to my Android-newbie eye that a lot of @NeedsTest annotations request a test that validates some UI flow, testable with Espresso. Examples: https://github.com/ankidroid/Anki-Android/blob/main/AnkiDroid/src/main/java/com/ichi2/anki/InitialActivity.kt#L188-L194

Or am I misunderstanding something?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Hello 👋, this issue has been opened for more than 3 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Aug 3, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 10, 2023
@david-allison david-allison reopened this Aug 21, 2023
@david-allison david-allison added Keep Open avoids the stale bot and removed Stale labels Aug 21, 2023
mikehardy pushed a commit that referenced this issue Aug 25, 2023
…4291)

* refactor: separate logic from dependencies by extracting easy to test class FlagToDisplay
* test: cover FLAG_NONE case
* test: Resolve @NeedsTest("is hidden if flag is on app bar")
* test: Resolve @NeedsTest("is not hidden if flag is not on app bar")
* test: Resolve @NeedsTest("is not hidden if flag is on app bar and fullscreen is enabled")
* test: Remove redundant test, as already covered by other tests
* refactor: simplify now tested logic
* test: remove @RunWith to fix Roboelectric Exception
* refactor: add more meaning to field by renaming to actualFlag
* refactor: revert accidental auto formatting
* refactor: add own copyright

---------

Co-authored-by: Paul Tietz <pati@aprixon.de>
@roryjones123
Copy link
Contributor

roryjones123 commented Aug 28, 2023

Hey :), I've written a test for:

protected open fun editCard(fromGesture: Gesture? = null) {

to cover that inverse animations are being correctly assigned from gestures.

#14337

n1snt pushed a commit to n1snt/Anki-Android that referenced this issue Sep 7, 2023
…er.kt (ankidroid#14291)

* refactor: separate logic from dependencies by extracting easy to test class FlagToDisplay
* test: cover FLAG_NONE case
* test: Resolve @NeedsTest("is hidden if flag is on app bar")
* test: Resolve @NeedsTest("is not hidden if flag is not on app bar")
* test: Resolve @NeedsTest("is not hidden if flag is on app bar and fullscreen is enabled")
* test: Remove redundant test, as already covered by other tests
* refactor: simplify now tested logic
* test: remove @RunWith to fix Roboelectric Exception
* refactor: add more meaning to field by renaming to actualFlag
* refactor: revert accidental auto formatting
* refactor: add own copyright

---------

Co-authored-by: Paul Tietz <pati@aprixon.de>
mikunimaru pushed a commit to mikunimaru/Anki-Android that referenced this issue Sep 19, 2023
…er.kt (ankidroid#14291)

* refactor: separate logic from dependencies by extracting easy to test class FlagToDisplay
* test: cover FLAG_NONE case
* test: Resolve @NeedsTest("is hidden if flag is on app bar")
* test: Resolve @NeedsTest("is not hidden if flag is not on app bar")
* test: Resolve @NeedsTest("is not hidden if flag is on app bar and fullscreen is enabled")
* test: Remove redundant test, as already covered by other tests
* refactor: simplify now tested logic
* test: remove @RunWith to fix Roboelectric Exception
* refactor: add more meaning to field by renaming to actualFlag
* refactor: revert accidental auto formatting
* refactor: add own copyright

---------

Co-authored-by: Paul Tietz <pati@aprixon.de>
@WPum
Copy link
Contributor

WPum commented Mar 22, 2024

I'm working on @NeedsTest("constant values are unique").

@neeldoshii
Copy link
Contributor

I'm taking up this

@NeedsTest("ensure this is called when the activity ends")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue! Keep Open avoids the stale bot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants