Skip to content

Adding Hilt#16346

Merged
irfano merged 10 commits intofeature/dagger-to-hiltfrom
feature/add-hilt
Apr 21, 2022
Merged

Adding Hilt#16346
irfano merged 10 commits intofeature/dagger-to-hiltfrom
feature/add-hilt

Conversation

@irfano
Copy link
Copy Markdown
Member

@irfano irfano commented Apr 17, 2022 •
edited
Loading

This is the last PR of hilt migration (parent PR #16143). In this PR,

  • Added hilt dependency,
  • Made dagger modules compatible with Hilt,
  • Added WordPressRelease for the release application to be able to use @HiltAndroidApplication. HiltAndroidApplication annotation is required for WordPressDebug and WordPressRelease, but we can't use it with the test application,
  • Made UI tests compatible with Hilt.

To test:
There is no user-facing change. Build the app with different variants to see if it's still buildable and tests are working.

Regression Notes

  1. Potential unintended areas of impact
    Some tests might be broken. Debug and release builds might work wrong.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    I run current tests. I built the debug app. I relied on automated tests for release build.

  3. What automated tests I added (or what prevented me from doing so)
    There is no new feature. No tests are added.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 17, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Apr 17, 2022

Found 1 violations:

The PR caused the following dependency changes:

-+--- com.google.dagger:dagger:2.41
-|    \--- javax.inject:javax.inject:1
 +--- com.google.dagger:dagger-android-support:2.41
-|    \--- com.google.dagger:dagger:2.41 (*)
+|    \--- com.google.dagger:dagger:2.41
+|         \--- javax.inject:javax.inject:1
+\--- com.google.dagger:hilt-android:2.41
+     +--- com.google.dagger:dagger:2.41 (*)
+     +--- com.google.dagger:dagger-lint-aar:2.41
+     +--- com.google.dagger:hilt-core:2.41
+     |    +--- com.google.dagger:dagger:2.41 (*)
+     |    +--- com.google.code.findbugs:jsr305:3.0.2
+     |    \--- javax.inject:javax.inject:1
+     +--- com.google.code.findbugs:jsr305:3.0.2
+     +--- androidx.activity:activity:1.3.1 (*)
+     +--- androidx.annotation:annotation:1.2.0
+     +--- androidx.fragment:fragment:1.3.6 (*)
+     +--- androidx.lifecycle:lifecycle-common:2.3.1 (*)
+     +--- androidx.lifecycle:lifecycle-viewmodel:2.3.1 (*)
+     +--- androidx.lifecycle:lifecycle-viewmodel-savedstate:2.3.1 (*)
+     +--- androidx.savedstate:savedstate:1.1.0 (*)
+     +--- javax.inject:javax.inject:1
+     \--- org.jetbrains.kotlin:kotlin-stdlib:1.5.32 -> 1.6.10 (*)

Please review and act accordingly

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 17, 2022

You can test the changes on this Pull Request by downloading the APKs:

@irfano irfano marked this pull request as ready for review April 18, 2022 12:37
@irfano irfano mentioned this pull request Apr 18, 2022
Merged
3 tasks
ravishanker
ravishanker approved these changes Apr 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ravishanker ravishanker left a comment

Choose a reason for hiding this comment

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

Great job 💯 🎉
Feel free to merge it

WordPress/src/androidTest/java/org/wordpress/android/InitializationRule.kt
import org.junit.runner.Description
import org.junit.runners.model.Statement

class InitializationRule : TestRule {
Copy link
Copy Markdown
Contributor

@ravishanker ravishanker Apr 21, 2022

Choose a reason for hiding this comment

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

Could this be named TestInitializationRule for better?

Copy link
Copy Markdown
Member Author

@irfano irfano Apr 21, 2022

Choose a reason for hiding this comment

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

InitializationRule doesn't initialize tests, it initializes AppInitializer. It is a TestRule that creates AppInitializer and run its init() function. Hilt is normally doing that in the application class, but in tests, we're doing it manually with InitializationRule. Other TestRule names in BaseTest are HiltAndroidRule, ActivityScenarioRule, WireMockRule. These also don't start with "Test".
If you still think it could be better to name it TestInitializationRule, we can do it. wdyt?

@irfano irfano merged commit a6fa473 into feature/dagger-to-hilt Apr 21, 2022
@irfano irfano deleted the feature/add-hilt branch April 21, 2022 22:38
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 26, 2022

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hichamboushaba hichamboushaba Awaiting requested review from hichamboushaba

1 more reviewer

@ravishanker ravishanker ravishanker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

[Type] Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@irfano @wpmobilebot @ravishanker