Increase testing coverage and maintainability of android manifest parsing logic #174070
Conversation
…ut of utils into task
… and add unit tests
There was a problem hiding this comment.
Code Review
This pull request is a great improvement. It refactors the Android manifest parsing logic for deeplinks into a dedicated, modern Gradle task, which significantly enhances maintainability and testability. The addition of a comprehensive test suite with a manifest builder is also excellent. My feedback focuses on further improving maintainability by leveraging Kotlin's features for immutability in your data classes, which is a best practice and aligns with the goals of this PR.
| @VisibleForTesting | ||
| var scheme: String?, | ||
| @VisibleForTesting | ||
| var host: String?, | ||
| @VisibleForTesting | ||
| var path: String?, | ||
| private var intentFilterCheck: IntentFilterCheck | ||
| @VisibleForTesting | ||
| var intentFilterCheck: IntentFilterCheck |
There was a problem hiding this comment.
These properties do not appear to be modified after the Deeplink object is created. You can make them immutable by using val instead of var. This improves maintainability, makes the class thread-safe, and also resolves the TODO on line 12.1
Since val properties are public by default, you can also remove the @VisibleForTesting annotation.
val scheme: String?,
val host: String?,
val path: String?,
val intentFilterCheck: IntentFilterCheckStyle Guide References
Footnotes
-
The Android Kotlin Style Guide recommends preferring
valovervarfor properties that are not reassigned after initialization to ensure immutability. ↩
| class IntentFilterCheck( | ||
| var hasAutoVerify: Boolean = false, | ||
| var hasActionView: Boolean = false, | ||
| var hasDefaultCategory: Boolean = false, | ||
| var hasBrowsableCategory: Boolean = false | ||
| ) { |
There was a problem hiding this comment.
This class can be converted to a data class with immutable val properties. This improves maintainability and makes instances of this class truly immutable.1
This change would require adjusting the instantiation logic in DeepLinkJsonFromManifestTaskHelper.kt to gather all boolean values before creating the IntentFilterCheck instance, which would also be an improvement.
| class IntentFilterCheck( | |
| var hasAutoVerify: Boolean = false, | |
| var hasActionView: Boolean = false, | |
| var hasDefaultCategory: Boolean = false, | |
| var hasBrowsableCategory: Boolean = false | |
| ) { | |
| data class IntentFilterCheck( | |
| val hasAutoVerify: Boolean = false, | |
| val hasActionView: Boolean = false, | |
| val hasDefaultCategory: Boolean = false, | |
| val hasBrowsableCategory: Boolean = false | |
| ) { |
Style Guide References
Footnotes
-
The Android Kotlin Style Guide recommends preferring
valovervarfor properties that are not reassigned after initialization to ensure immutability. Using adata classalso provides useful generated methods likeequals(),hashCode(), andtoString(). ↩
| val intentFilterCheck = IntentFilterCheck() | ||
| if (appLinkIntent.attribute("android:autoVerify") == MANIFEST_VALUE_TRUE) { | ||
| intentFilterCheck.hasAutoVerify = true | ||
| } | ||
|
|
||
| val actionItems: List<Node> = | ||
| appLinkIntent.children().filterIsInstance<Node>().filter { item -> | ||
| item.name() == "action" | ||
| } | ||
|
|
||
| intentFilterCheck.hasActionView = | ||
| actionItems.any { action -> | ||
| action.attribute(MANIFEST_NAME_KEY) == "android.intent.action.VIEW" | ||
| } | ||
| val categoryItems: List<Node> = | ||
| appLinkIntent.children().filterIsInstance<Node>().filter { item -> | ||
| item.name() == "category" | ||
| } | ||
| intentFilterCheck.hasDefaultCategory = | ||
| categoryItems.any { category -> category.attribute(MANIFEST_NAME_KEY) == "android.intent.category.DEFAULT" } | ||
| intentFilterCheck.hasBrowsableCategory = | ||
| categoryItems.any { category -> category.attribute(MANIFEST_NAME_KEY) == "android.intent.category.BROWSABLE" } |
There was a problem hiding this comment.
To support making IntentFilterCheck an immutable data class, you can refactor this block to first determine the values for its properties, and then create an immutable instance in a single call. This improves readability and maintainability by separating the logic for determining the checks from the object creation.1
val hasAutoVerify = appLinkIntent.attribute("android:autoVerify") == MANIFEST_VALUE_TRUE
val actionItems: List<Node> =
appLinkIntent.children().filterIsInstance<Node>().filter { item ->
item.name() == "action"
}
val hasActionView = actionItems.any { action ->
action.attribute(MANIFEST_NAME_KEY) == "android.intent.action.VIEW"
}
val categoryItems: List<Node> =
appLinkIntent.children().filterIsInstance<Node>().filter { item ->
item.name() == "category"
}
val hasDefaultCategory = categoryItems.any { category -> category.attribute(MANIFEST_NAME_KEY) == "android.intent.category.DEFAULT" }
val hasBrowsableCategory = categoryItems.any { category -> category.attribute(MANIFEST_NAME_KEY) == "android.intent.category.BROWSABLE" }
val intentFilterCheck = IntentFilterCheck(
hasAutoVerify = hasAutoVerify,
hasActionView = hasActionView,
hasDefaultCategory = hasDefaultCategory,
hasBrowsableCategory = hasBrowsableCategory
)Style Guide References
Footnotes
-
The Android Kotlin Style Guide recommends preferring
valovervarfor properties that are not reassigned after initialization to ensure immutability. ↩
Related to: #166550
Follow up to #173794 to implement suggested improvements and with strictly human authored tests changes.
Pre-launch Checklist
///).