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

Use JUnit5 on tests #11202

Merged
merged 1 commit into from
May 2, 2022
Merged

Use JUnit5 on tests #11202

merged 1 commit into from
May 2, 2022

Conversation

BrayanDSO
Copy link
Member

Pull Request template

Purpose / Description

Describe the problem or feature and motivation

Fixes

Fixes #11201

Approach

  1. Extract JUnit version to a constant, so all 3 testImplementations uses the same version
  2. Add JUnitPlatform use to build.gradle
  3. Fix imports to kotlin.test.junit5.JUnit5Asserter
  4. Fix AbstractFlashcardViewerTest.getSignalFromUrlTest
    • It is currently the only parameterized test that uses jupiter-params. It wasn't being discovered before, so it haven't raised any exception.
    • Now that it's discovered, it needed to be fixed

How Has This Been Tested?

gradlew lint+ jacocoUnitTestReport

Learning (optional, can help others)

https://docs.gradle.org/current/userguide/java_testing.html#using_junit5
https://stackoverflow.com/a/68934967

Android JUnit isn't officially updated yet (links taken from a 3rd party Android JUnit5 repo)

InstantTaskExecutorRule uses @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) -- why? (issuetracker.google.com)
Add support for JUnit 5 (issuetracker.google.com)
android/android-test#224

Checklist

Please, go through these checks before submitting the PR.

@BrayanDSO
Copy link
Member Author

BrayanDSO commented May 1, 2022

Now we can use junit parameterized tests, building some custom annotations later would be good IMO.

As an example here https://github.com/ankidroid/Anki-Android/pull/11202/files#diff-ec045306ef509765aeb114371f47e8e180c58c5ef7bfbe88f4bf51f7c0396378R255-R269 it could be passed the args directly to a annotation, so there is less boilerplate. Somewhat similar to python's pytest parameterized annotations

edit: https://www.baeldung.com/parameterized-tests-junit-5#8-custom-annotation can be an example

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.

This looks great - my comments are both about maybe taking it farther, that is not just Anki-Android but also api and lint-rules module

1. Extract JUnit version to a constant, so all 3 testImplementations uses the same version
2. Add JUnitPlatform use to `build.gradle`
3. Fix imports to `kotlin.test.junit5.JUnit5Asserter`
4. Fix `AbstractFlashcardViewerTest.getSignalFromUrlTest`
- It is currently the only parameterized test that uses jupiter-params. It wasn't being discovered before, so it haven't raised any exception.
- Now that it's discovered, it needed to be fixed
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.

did I just miss the junit in ext before? 😆 🤷
This looks great though, thanks for helping clean up the build files, been a while since I groomed them and entropy seemed to have taken over pretty quickly! Looking better now

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label May 1, 2022
@mikehardy mikehardy requested a review from david-allison May 1, 2022 21:50
@mikehardy mikehardy added the Review High Priority Request for high priority review label May 1, 2022
@mikehardy
Copy link
Member

@david-allison asking for high priority review since A) should be quick and B) blocks something else, I always try to clear blockers when possible. Thanks!

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.

LGTM. Thanks so much! Added one comment which has no action needed

@mikehardy - pinging in case you want to run this though your CI

@david-allison david-allison 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 May 1, 2022
@mikehardy
Copy link
Member

Nope - this one can't really have cross-contamination. The ones we have been unlucky on are when Kotlin vs Java null/non-null typings have changed while there's 10 other PRs in the queue. For any of the normal stuff that can't cross-contaminate, I'm all for hitting the button personally

@mikehardy mikehardy merged commit 8f9f8c2 into ankidroid:main May 2, 2022
@github-actions github-actions bot removed Review High Priority Request for high priority review Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels May 2, 2022
@github-actions github-actions bot added this to the 2.16 release milestone May 2, 2022
@BrayanDSO BrayanDSO deleted the JUnit5 branch May 2, 2022 00:50
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.

Update to JUnit 5
3 participants