[Lint Warnings] Reconfigure & Add Baseline#18186
Merged
Conversation
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
15 tasks
Contributor
|
| App Name | WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr18186-771bce9 | |
| Commit | 771bce9 | |
| Direct Download | wordpress-prototype-build-pr18186-771bce9.apk |
Contributor
|
| App Name | Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr18186-771bce9 | |
| Commit | 771bce9 | |
| Direct Download | jetpack-prototype-build-pr18186-771bce9.apk |
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
77f2399 to
6216872
Compare
… into analysis/reconfigure-and-add-baseline
… into analysis/reconfigure-and-add-baseline
… into analysis/reconfigure-and-add-baseline
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! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


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
500existing 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):
Reconfigure Lint (Narrow Down):
Reconfigure Lint (Reformat):
Reconfigure Lint (Ignore):
MissingFirebaseInstanceTokenRefresh#18185Warnings Suppression List:
AppBundleLocaleChanges#18188Warnings Resolution List:
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:
WordPressandJetpackapps, on all, theJalapeno,WasabiandVanillaproduct flavors, on both, theDebugandReleasebuild types, and see if everything is working as expected.Merging Instructions:
trunkand wait for CI to run again all its checks.[Status] Not Ready for Mergelabel and merge totrunk.Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
To testsection above.What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.