Skip to content

[Lint Warnings] Reconfigure & Add Baseline#18186

Merged
AjeshRPai merged 28 commits intotrunkfrom
analysis/reconfigure-and-add-baseline
Mar 31, 2023
Merged

[Lint Warnings] Reconfigure & Add Baseline#18186
AjeshRPai merged 28 commits intotrunkfrom
analysis/reconfigure-and-add-baseline

Conversation

@ParaskP7
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 commented Mar 28, 2023

Parent #18142
Closes #18161

This PR changes Lint's (outdated) configuration (lint/lint.xml), enables all Lint warnings as errors (warningsAsErrors = true) and adds a new Lint baseline (lint/baseline.xml).

FYI: With this change this project is effectively becomes more strict on Lint by not allowing any more warnings entering the codebase. Also, with that, a baseline of about 500 existing Lint warnings is created, which we are going to deal with (resolve/suppress) at a later point in time, most probably via #18162.


Reconfigure Lint (Remove):

  1. Remove unnecessary icon missing density folder info config.
  2. Remove unnecessary rtl symmetry warning config.
  3. Remove unnecessary use sparse arrays warning config.
  4. Remove unnecessary label for ignore config.
  5. Remove unnecessary old target api ignore config.
  6. Remove use app tint ignore config and use default.
  7. Remove unnecessary invalid package config.
  8. Remove unnecessary unused res ignore config for gradle caches.
  9. Remove unnecessary invalid fragment version ignore config.

Reconfigure Lint (Narrow Down):

  1. Narrow down ignore config for icon dip size.
  2. Narrow down error config for unused resources.
  3. Narrow down error config for unused resources for generated.
  4. Ignore unused resources lint warnings for generated.

Reconfigure Lint (Reformat):

  1. Reorder lint config issues based on lint category.
  2. Add lint category on lint config issues.
  3. Add another layer of lint category on lint config issues.

Reconfigure Lint (Ignore):

  1. Ignore missing firebase instance token refresh lint warning. -> Lint Warnings - Resolve MissingFirebaseInstanceTokenRefresh #18185
  2. Ignore gradle dependency lint warnings.
  3. Ignore icon missing density folder lint warnings.
  4. Ignore trust all x509 trust manager lint warnings.

Warnings Suppression List:

  1. Suppress app bundle locale changes lint warnings. -> Lint Warnings - Resolve AppBundleLocaleChanges #18188

Warnings Resolution List:

  1. Resolve ignore without reason lint warning.

PS: @AjeshRPai I added you as the main reviewer, randomly, since I just wanted someone from the WordPress team to be aware of and sign-off on that change for WPAndroid. I also added the @wordpress-mobile/apps-infrastructure team, but this in done only for monitoring purposes, as such, I am not expecting any active review from that team. Thus, feel free to merge this PR if you deem so.


To test:

  1. There is nothing much to test here.
  2. Verifying that all the CI checks are successful should be enough.
  3. However, if you want to be thorough about reviewing this change, you could try and run Lint locally on both, the WordPress and Jetpack apps, on all, the Jalapeno, Wasabi and Vanilla product flavors, on both, the Debug and Release build types, and see if everything is working as expected.

Merging Instructions:

  1. Wait for this PR to be reviewed, tested and approved.
  2. Merge this PR with latest trunk and wait for CI to run again all its checks.
  3. If CI checks are all ✅ then remove the [Status] Not Ready for Merge label and merge to trunk.
  4. Else, suppress or resolve any new Lint warnings that might appear and try this whole process again.

Regression Notes

  1. Potential unintended areas of impact

    • N/A
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • See To test section above.
  3. 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.

ParaskP7 added 10 commits March 28, 2023 14:12
This configuration is unnecessary since removing it will just make
'IconMissingDensityFolder' a warning and add 2+ such warnings to the
overall number of warnings (currently about 450 in total).

FYI: The default severity of 'IconMissingDensityFolder' is warning.
Link: http://tools.android.com/tips/lint-checks
This configuration is unnecessary since another such 'RtlSymmetry' error
configuration has been defined further below, which overrides this prior
such warning configuration.

FYI: The default severity of 'RtlSymmetry' is warning.
Link: http://tools.android.com/tips/lint-checks
This configuration is unnecessary as it doesn't affect the final Lint
result, or report numbers, that is, after it is being removed, on both
the debug or release variants, and on both WordPress or Jetpack apps.

FYI: The default severity of 'UseSparseArrays' is warning.
Link: http://tools.android.com/tips/lint-checks
The 'IconDipSize' error is only applicable for the 'drawable-hdpi'
related resource and not for the rest 2 of them, the 'drawable-xhdpi'
and 'drawable-xxhdpi' (there are 3 such resources in total). As such, it
is better to be explicit on that to narrow down the specific error that
is being ignored.

FYI: The default severity of 'IconDipSize' is warning.
Link: http://tools.android.com/tips/lint-checks
This configuration is unnecessary since removing it will just make
'LabelFor' a warning and add 10+ such warnings to the
overall number of warnings (currently about 450 in total).

FYI: The default severity of 'LabelFor' is warning.
Link: http://tools.android.com/tips/lint-checks
This configuration is unnecessary as it doesn't affect the final Lint
result, or report numbers, that is, after it is being removed, on both
the debug or release variants, and on both WordPress or Jetpack apps.

FYI: The default severity of 'OldTargetApi' is warning.
Link: http://tools.android.com/tips/lint-checks
Removing it will make 'UseAppTint' an error and add 36 new errors.

However, that's totally okay. Those errors will be later on moved into
the new Lint 'baseline.xml' file, which will make sure that Lint builds
successfully afterwards, but also, that no new such errors are allowed
to enter the codebase.

FYI: The default severity of 'UseAppTint' is error.
Link: http://tools.android.com/tips/lint-checks (N/A)
This configuration is unnecessary as it doesn't affect the final Lint
result, or report numbers, that is, after it is being removed, on both
the debug or release variants, and on both WordPress or Jetpack apps.

FYI: The default severity of 'InvalidPackage' is error.
Link: http://tools.android.com/tips/lint-checks
The 'UnusedResources' error for the 'editor' module, is only applicable
for the 'drawable', 'drawable-hdpi', 'layout/alert_create_link.xml',
'layout/alert_image_options.xml', 'values/colors.xml',
'values/wp_colors.xml', 'values/dimens.xml' and 'values/styles.xml'
related resources and not for the rest of them. As such, it is better to
be explicit on that to narrow down the specific error that is being
ignored.

Also, for some of those, 'UnusedResources' was suppressed closer to res
source, that is, instead of it being on the 'lint.xml' configuration
file, and that, in order to help fixing those errors easier later on.

FYI: The default severity of 'UnusedResources' is warning.
Link: http://tools.android.com/tips/lint-checks
This configuration is unnecessary as it doesn't affect the final Lint
result, or report numbers, that is, after it is being removed, on both
the debug or release variants, and on both WordPress or Jetpack apps.

FYI: The default severity of 'UnusedResources' is warning.
Link: http://tools.android.com/tips/lint-checks
@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Mar 28, 2023

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr18186-771bce9
Commit771bce9
Direct Downloadwordpress-prototype-build-pr18186-771bce9.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented Mar 28, 2023

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr18186-771bce9
Commit771bce9
Direct Downloadjetpack-prototype-build-pr18186-771bce9.apk
Note: Google Login is not supported on these builds.

ParaskP7 added 13 commits March 28, 2023 15:48
The 'UnusedResources' error is only applicable for the 'google-services'
related 'values/values.xml' resource and not for the rest of them. As
such, it is better to be explicit on that to narrow down the specific
error that is being ignored.

FYI: The default severity of 'UnusedResources' is warning.
Link: http://tools.android.com/tips/lint-checks
This configuration is unnecessary as it doesn't affect the final Lint
result, or report numbers, that is, after it is being removed, on both
the debug or release variants, and on both WordPress or Jetpack apps.

FYI: The default severity of 'InvalidFragmentVersionForActivityResult'
is unknown.
Link: http://tools.android.com/tips/lint-checks (N/A)
With that change all Lint warnings are now treated as errors. As such,
it is no longer necessary to have an explicit warnings-as-errors
configuration within the 'lint.xml' file. Now, the only necessary
configuration within this 'lint.xml' file is related to specific errors
as a whole (like 'MissingTranslation', 'ExtraTranslation' and
'PluralsCandidate') or ignoring a set of errors based on a path or a
regular expression (like 'IconDipSize', 'UnusedResources' and 'Typos').

FYI: The current total amount of errors are about 500.
Link: https://developer.android.com/reference/tools/gradle-api/
4.1/com/android/build/api/dsl/LintOptions
This is done because the (to be generated) Lint related 'baseline.xml'
will also live within this newly created 'lint' config folder, just like
it is currently done for 'detekt'.
This Lint baseline suppresses the existing warnings-as-errors issues
(about 500 in total) to have Lint running as if the codebase has zero
issues, but still failing the build on new warnings, just like it used
to fail the build on new errors. This helps to start utilising the new
warnings-as-errors Lint configuration immediately without having to
worry about any existing warnings.

Any new warnings that will be added to the codebase right after will be
reported and the build will fail accordingly.

FYI: Actual number of current warnings per app/variant:
- WordPress/Debug:   486
- WordPress/Release: 481
- Jetpack/Debug:     492
- Jetpack/Release:   487

PS: I excluded the further breakdown of Debug to the Wasabi and Jalapeno
variant as the only difference there is the extra generated
'MissingFirebaseInstanceTokenRefresh' issue, which is variant specific.
This Lint warning is moved within lint config and removed from Lint's
baseline in order to make it app, product flavor and build type agnostic
as it was otherwise polluting the baseline with 6 duplicate warnings,
per app (WordPress and Jetpack), per product flavor (Jalapeno, Wasabi
and Vanilla) and build type (Debug and Release).

Related: Lint Warnings - Resolve 'MissingFirebaseInstanceTokenRefresh'
- #18185
This Lint warning is moved and ignored within lint config, then removed
from Lint's baseline. This is done because:
1. This warnings can appear in an ad-hoc manner and fail the build
locally or on CI at any point of time, thus making them
non-deterministic.
2. Dependabot takes care of dependency updates anyway.
In a previous 45ea4ad commit, this
'UnusedResources' error got narrowed down from the general
'**/generated/**' regular expression to become only applicable for the
'google-services' related 'values/values.xml' resource.

Although this was enough when building locally, on CI there was an issue
with another such 'google-services' related resource, the
'xml/global_tracker.xml' resource. This commit add that resource
to the ignore list of unused resources.

CI: https://buildkite.com/automattic/wordpress-android/builds/10254
This Lint warnings are moved and ignored within lint config, then
removed from Lint's baseline. This is done in order to make the
suppression more robust as although is seems to be working locally, it
actually failing when running on CI.

CI: https://buildkite.com/automattic/wordpress-android/builds/10254
This Lint warnings are being suppressed closer to source and removed
from Lint's baseline. This is done in order to make the suppression more
robust as although is seems to be working locally, it actually failing
when running on CI.

CI: https://buildkite.com/automattic/wordpress-android/builds/10254
Related: Lint Warnings - Resolve 'AppBundleLocaleChanges'
- #18188
This Lint warnings are moved and ignored within lint config, then
removed from Lint's baseline. This is done because those are dependency
related warnings that we have no control over and can't resolve anyway.

Explanation: "checkClientTrusted/checkServerTrusted is empty, which
could cause insecure network traffic due to trusting arbitrary TLS/SSL
certificates presented by peers."
This test was ignored as part of the below PR but hasn't been addressed
yet.

PS: [UI Tests] Ignored (disabled) 'e2ePublishFullPost' UI test.
- #17408
@ParaskP7 ParaskP7 force-pushed the analysis/reconfigure-and-add-baseline branch from 77f2399 to 6216872 Compare March 28, 2023 13:49
@ParaskP7 ParaskP7 requested review from a team and AjeshRPai March 28, 2023 14:15
@ParaskP7 ParaskP7 marked this pull request as ready for review March 28, 2023 14:16
@ParaskP7 ParaskP7 requested a review from a team as a code owner March 28, 2023 14:16
Copy link
Copy Markdown
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

Hey @ParaskP7 ,

Everything looks good to me. I ran the lint locally. It's Successful. 👍🏼 . I am going ahead and merging this PR 👍🏼

@AjeshRPai AjeshRPai merged commit 3e48aa4 into trunk Mar 31, 2023
@AjeshRPai AjeshRPai deleted the analysis/reconfigure-and-add-baseline branch March 31, 2023 11:30
@ParaskP7
Copy link
Copy Markdown
Contributor Author

Awesome, thank you so much for reviewing, testing and merging this @AjeshRPai , you rock! 🙇 ❤️ 🚀


🎉 No more new Lint warnings will enter our codebase, I'll announce that change in a P2 post shortly! 🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lint Warnings - Reconfigure & Add Baseline

3 participants