[Compile Warnings As Errors] WordPress Module - Resolve Test Warnings by ParaskP7 · Pull Request #17300 · wordpress-mobile/WordPress-Android

ParaskP7 · 2022-10-11T15:45:07Z

Parent: #17173
Partially Closes: #17181

This PR resolves one part of all Test related warnings for the WordPress module:

Deprecated Related (80)

58 x 'XYX' is deprecated. Deprecated in Java
2 x 'xyz' is deprecated. Use xyz instead.
20 x 'Xyz' is deprecated. This class is being refactored, if you implement any change, please also update xyz

Type Related (8)

3 x Type mismatch: inferred type is Xyz? but Xyz was expected
4 x Unnecessary non-null assertion (!!) on a non-null receiver of type Xyz
1 x Unnecessary safe call on a non-null receiver of type Xyz. This expression will have nullable type in future releases

Cast Related (6)

4 x Unchecked cast: Xyz to Xyz
2 x This cast can never succeed

When Related (1)

1 x Non exhaustive 'when' statements on xyz will be prohibited in 1.7, add 'Xyz', 'Xyz' branch(es) or 'else' branch instead

Other Related (23)

5 x Parameter 'xyz' is never used
2 x Check for instance is always 'xyz'
15 x Name contains characters which can cause problems on Windows: "
1 x Name shadowed: fileName

Warnings Resolution List:

Warnings Suppression List:

Refactor List:

PS: @AjeshRPai I added you as the main reviewer, that is, in addition to @wordpress-mobile/apps-infrastructure team itself, but randomly, since I want someone from the WordPress Android team to primarily sign-off on that change. 🥇

FYI: I am going to randomly add more of you in those PRs that will follow, just so you become more aware of this change and how close we are on enabling allWarningsAsErrors by default everywhere. 🎉

To test:

There is nothing much to test here.
Verifying that all the CI checks are successful should be enough.

Regression Notes

Potential unintended areas of impact

N/A

What I did to test those areas of impact (or what existing automated tests I relied on)

See To test section above.

What automated tests I added (or what prevented me from doing so)

N/A

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.

Warning Messages: - "'assertThat(T!, Matcher<in T!>!): Unit' is deprecated. Deprecated in Java" - "'assertThat(String!, T!, Matcher<in T!>!): Unit' is deprecated. Deprecated in Java" Using 'Assertions.assertThat' from the 'org.assertj.core.api' package, instead of 'Assert.assertThat' from the 'org.junit' package is the recommended action to resolve this kind of deprecated warnings.

In addition to that, this commit also replaces the 'runBlockingTest' block, from the 'kotlinx.coroutines.test' package, with the 'test' block, from [CoroutinesUtils], that is using 'runBlocking' underneath anyway. This was done to make tests simpler to reason about and easier to be migrated to Coroutines version 1.6, which completely changes how testing with Coroutines works. The only tests that are still using 'runBlockingTest', and doing that purposefully are the below: - [BatchModerateCommentsUseCaseTest] - [ModerateCommentsWithUndoUseCaseTest] - [PaginateCommentsUseCaseTest] - [QRCodeAuthViewModelTest] - [SnackbarSequencerConcurrentTest] - [UnifiedCommentListViewModelTest] Also, the 'coroutineScope' rule annotations, that were pointing to [MainCoroutineScopeRule] were removed as well, that is, where they were indeed unnecessary.

Warning Messages: "'Assert' is deprecated. Deprecated in Java" Using 'Assertions.assertThat' from the 'org.assertj.core.api' package, instead of 'Assert.assertEquals/False/True' from the 'junit.framework' package is the recommended action to resolve this kind of deprecated warnings.

Warning Message: "'allowScanningByMediaScanner(): Unit' is deprecated. Deprecated in Java" These deprecated warnings are suppressed, that is, instead of being resolved, since a resolution would require a proper migration. For more info see: - android.app.DownloadManager.Request.allowScanningByMediaScanner (Doc): https://developer.android.com/reference/android/app/ DownloadManager.Request#allowScanningByMediaScanner()

Warning Messages: "'none(): ExpectedException!' is deprecated. Deprecated in Java" Also, as part of this commit, a few other minor warnings, including typos, got resolved too.

This is done for consistency purposes only, as most of the other usages of [InstantTaskExecutorRule] is on multiline instead on a single line.

Warning Messages: - "'NetworkInfo' is deprecated. Deprecated in Java" - "'getter for activeNetworkInfo: NetworkInfo?' is deprecated. Deprecated in Java" - "'getter for isConnected: Boolean' is deprecated. Deprecated in Java" These deprecated warnings are suppressed, that is, instead of being resolved, since a resolution would require a proper migration. For more info see: - Compiler Warnings as Errors - WordPress Module - Connectivity Manager & Network Info #17230 (Issue): #17230

Warning Messages: - "'PhotoPickerViewModel' is deprecated. This class is being refactored, if you implement any change, please also update {@link org.wordpress.android.ui.mediapicker.MediaPickerViewModel}" - "'DeviceMediaListBuilder' is deprecated. This class is being refactored, if you implement any change, please also update {@link org.wordpress.android.ui.mediapicker.loader.DeviceListBuilder}" - "'PhotoPickerItem' is deprecated. This class is being refactored, if you implement any change, please also update {@link org.wordpress.android.ui.mediapicker.MediaItem}" - "'PhotoPickerUiItem' is deprecated. This class is being refactored, if you implement any change, please also update {@link org.wordpress.android.ui.mediapicker.MedaPickerUiItem}" These deprecated warnings are suppressed, that is, instead of being resolved, since a resolution would require a proper migration. Also, as part of this commit, a few other minor warnings, including imports, got resolved too. Example Explanation: See KDoc on 'PhotoPickerViewModel' class. "This class is being refactored, if you implement any change, please also update {@link org.wordpress.android.ui.mediapicker .MediaPickerViewModel}"

Warning Message: "Type mismatch: inferred type is Xyz? but Xyz was expected"

Warning Message: "Unnecessary non-null assertion (!!) on a non-null receiver of type Xyz"

Warning Message: "Unnecessary safe call on a non-null receiver of type Xyz. This expression will have nullable type in future releases"

Warning Message: "Unchecked cast: Xyz to Xyz" These type warnings are suppressed, that is, instead of being resolved, since a resolution would require a proper investigation. As such, it might be best to ignore those as out of scope, for now, and so as to not introduce any breaking changes to these tests overall.

Warning Message: "This cast can never succeed" These type warnings are suppressed, that is, instead of being resolved, since a resolution would require a proper investigation. As such, it might be best to ignore those as out of scope, for now, and so as to not introduce any breaking changes to these tests overall.

Warning Message: "Non exhaustive 'when' statements on enum will be prohibited in 1.7, add 'Xyz', 'Xyz' branch(es) or 'else' branch instead" Also, as part of this commit, one other minor warning got resolved too.

Warning Message: "Check for instance is always 'false'" Also, as part of this commit, one other minor warning got resolved too.

Warning Message: "Name contains characters which can cause problems on Windows: " Renaming '"xyz"' to ''xyz'' resolves this kind of 'name character' warnings. Also, as part of this commit, a few other minor warnings, including imports, got resolved too.

Warning Message: "Name shadowed: xyz" Also, as part of this commit, a couple of other minor warnings got resolved too.

wpmobilebot · 2022-10-11T16:01:11Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17300-42b38e8.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`42b38e8`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2022-10-11T16:01:28Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17300-42b38e8.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`42b38e8`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

AjeshRPai

Thanks for providing me with all the details for the review in the commit messages. 🙌🏼

I have tested the PR by going through the changes in every commit and reading the reference links mentioned in the commit messages and everything LGTM. And as the CI passes 🟢 , its a 👍🏼 from me.

I have left a ❓ for you to address. Since that's non-blocking. Am going to approve but not merge the PR so that anyone from @apps-infrastructure can have a look. Please feel free to merge the PR at your convenience.

AjeshRPai · 2022-10-12T08:51:30Z

...ress/src/test/java/org/wordpress/android/ui/comments/usecases/PaginateCommentsUseCaseTest.kt

 import org.junit.Test
 import org.mockito.Mock
-import org.mockito.Mockito
+import org.mockito.Mockito.*


Hey @ParaskP7 👋🏼
❓ Is the wild cart import necessary?.

👋 @AjeshRPai and thanks for noticing and hence the comment! 🙇

Actually no, I can replace that with import org.mockito.Mockito.'when', maybe that's better, although, to be honest I don't like the 'when' import syntax and in general us needing this 'when' syntax in tests. PS: This * import was done automatically for me by AS, when I clicked on optimizing the import.

Since, I am not seeing any such import org.mockito.Mockito.* imports for Mockito in our project, I'll go ahead and make this change, especially since we generally don't like * imports. 👍

FYI: This is now done: 42b38e8

Thanks, @ParaskP7 👍🏼 .

Also, as part of this commit, a few other minor warnings got resolved too.

ParaskP7 · 2022-10-12T09:58:48Z

Once more, thank you so much for reviewing this PR @AjeshRPai, you are being most helpful! 🙇 ❤️ 🚀

ParaskP7 added 19 commits October 11, 2022 16:55

Refactor: Replace hamcrest matchers with assertj assertions for test

f24421a

Analysis: Resolve none expected exception test deprecated warnings

a1c9e1e

Warning Messages: "'none(): ExpectedException!' is deprecated. Deprecated in Java" Also, as part of this commit, a few other minor warnings, including typos, got resolved too.

Refactor: Split rule and jvm field lines for instant task executor rule

d01fccd

This is done for consistency purposes only, as most of the other usages of [InstantTaskExecutorRule] is on multiline instead on a single line.

Analysis: Resolve type mismatch test type warnings

6676105

Warning Message: "Type mismatch: inferred type is Xyz? but Xyz was expected"

Analysis: Resolve unnecessary non-null assertion test type warnings

2e09cbd

Warning Message: "Unnecessary non-null assertion (!!) on a non-null receiver of type Xyz"

Analysis: Resolve unnecessary safe call test type warning

632adcc

Warning Message: "Unnecessary safe call on a non-null receiver of type Xyz. This expression will have nullable type in future releases"

Analysis: Resolve non exhaustive when statements test when warning

7b69170

Warning Message: "Non exhaustive 'when' statements on enum will be prohibited in 1.7, add 'Xyz', 'Xyz' branch(es) or 'else' branch instead" Also, as part of this commit, one other minor warning got resolved too.

Refactor: Replace for each indexed with for each for stats utils

c740332

Analysis: Resolve check for instance test warnings

4666d5d

Warning Message: "Check for instance is always 'false'" Also, as part of this commit, one other minor warning got resolved too.

Analysis: Resolve name shadowed ui test warning

6cc1d05

Warning Message: "Name shadowed: xyz" Also, as part of this commit, a couple of other minor warnings got resolved too.

ParaskP7 added Tooling Tech Debt labels Oct 11, 2022

ParaskP7 added this to the 21.0 milestone Oct 11, 2022

ParaskP7 requested review from a team and AjeshRPai October 11, 2022 15:45

ParaskP7 requested a review from a team as a code owner October 11, 2022 15:45

ParaskP7 self-assigned this Oct 11, 2022

This was referenced Oct 11, 2022

Compiler Warnings as Errors - WordPress Module #17181

Closed

Compiler Warnings as Errors - Resolve/Suppress All Warnings #17173

Closed

ParaskP7 added Tooling and removed Tooling labels Oct 11, 2022

ParaskP7 added Tooling Tech Debt and removed Tooling Tech Debt labels Oct 11, 2022

AjeshRPai approved these changes Oct 12, 2022

View reviewed changes

Refactor: Replace mockito star import with more specific when import

42b38e8

Also, as part of this commit, a few other minor warnings got resolved too.

ParaskP7 added Tooling Tech Debt and removed Tooling Tech Debt labels Oct 12, 2022

ParaskP7 merged commit a53f95a into trunk Oct 12, 2022

ParaskP7 deleted the analysis/wordpress-test-all-warnings branch October 12, 2022 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compile Warnings As Errors] WordPress Module - Resolve Test Warnings#17300

[Compile Warnings As Errors] WordPress Module - Resolve Test Warnings#17300
ParaskP7 merged 20 commits intotrunkfrom
analysis/wordpress-test-all-warnings

ParaskP7 commented Oct 11, 2022

Uh oh!

wpmobilebot commented Oct 11, 2022 •

edited

Loading

Uh oh!

wpmobilebot commented Oct 11, 2022 •

edited

Loading

Uh oh!

AjeshRPai left a comment

Uh oh!

AjeshRPai Oct 12, 2022

Uh oh!

ParaskP7 Oct 12, 2022

Uh oh!

ParaskP7 Oct 12, 2022

Uh oh!

AjeshRPai Oct 12, 2022

Uh oh!

ParaskP7 commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ParaskP7 commented Oct 11, 2022

Regression Notes

Uh oh!

wpmobilebot commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AjeshRPai left a comment

Choose a reason for hiding this comment

Uh oh!

AjeshRPai Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

AjeshRPai Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

ParaskP7 commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wpmobilebot commented Oct 11, 2022 •

edited

Loading

wpmobilebot commented Oct 11, 2022 •

edited

Loading