Introduce new platform parameter system infrastructure #5725
Conversation
There's a lot left to do in the way of integration and testing, plus a script that needs to be introduced. This is meant to be a first step toward a much more robust and future-proof parameter system.
|
@Rd4dev and @adhiamboperes I'd really like your thoughts on this initial approach for a reinventing of the platform parameter system. It's as much as I could get done today, but I'm really hoping it will serve as a significant foundation for both the testing work being done in #5565 and what we need to do for the rest of #5345. At a high-level, the plan is:
A lot of files will eventually need to be updated, plus the old system torn out. Would be great to get your thoughts of progress so far. Note that for testing I plan to update |
|
@BenHenning, actually it took me some time to understand the sync race issue, but I was able to get a better picture along the way. The restructured implementation would be very helpful to have a more organized approach, with works #5565, #5345. And I’m in favor of having a dedicated debug implementation.
But I was not able to fully understand the testing plan for simulating an app restart. Is it intended for all platform parameter test scenarios? or is it specifically for validating the new system’s cache storage and retention? If it's the latter, then what I was considering might not be relevant. But if not, I thought we could update the database before Dagger initialization in the test rule’s apply() method, based on test annotations, ensuring the updated values would already be picked up in the platform parameter controller during initialization. Wouldn't that be possible? Would it be possible to clarify the testing overrides a bit more, particularly how the app restart simulation should be carried forward in #5565? |
This introduces module generation, feature flag & platform parameter definitions, working implementations to the parameter system itself, plus demonstrated and verified support for overrides in tests and in the main app binary. There are still a lot of broken tests that need fixing, and the app itself is integrating the system incorrectly due to a very early platform parameter injection requirement that was missed. The initialization flow from the app startup through splash will need to be redone a bit in order to hook up the new system correctly, but this commit proves the system should be viable.
|
Thanks @Rd4dev, I appreciate the read-through and follow-up. A lot has changed since that comment as I worked through trying to make this work. :) It is, admittedly, rather complex to get the initialization flows correct (which affects both the app and all tests that interact with code which use parameters/flags, or need to override them). I think I figured out a working model. Please look at the following files in the latest changes:
Note that application initialization has been hacked into As for how this affects testing, my thoughts are:
This PR still needs a lot of work to get tests passing and adding new tests (beyond the initialization revamp mentioned above), but I think that this at least proves the viability of the new approach. I think we can use this as the basis now for both finishing your test PR and building a more clearly defined foundation for the dashboard project. |
BenHenning
changed the title
BenHenning
changed the title
Some of the more affected targets (such as for feature flag logging) have been directly updated and verified to pass. Logging itself has also been fixed by introducing some new sync status-specific generation support.
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 4368 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 7211 bytes (Added) Method count: 260308 (old), 260851 (new), 543 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6832 (old), 6832 (new), 0 (No change)
Lesson assets: 111 (old), 113 (new), 2 (Added):
AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 4368 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 8521 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 8922 bytes (Added) Method count: 115790 (old), 116022 (new), 232 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5800 (old), 5800 (new), 0 (No change)
Lesson assets: 111 (old), 114 (new), 3 (Added):
AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 8525 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 8668 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 13 KiB (Added) Method count: 115796 (old), 116028 (new), 232 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5800 (old), 5800 (new), 0 (No change)
Lesson assets: 111 (old), 114 (new), 3 (Added):
AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 8668 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 8738 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 13 KiB (Added) Method count: 115796 (old), 116028 (new), 232 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5800 (old), 5800 (new), 0 (No change)
Lesson assets: 111 (old), 114 (new), 3 (Added):
AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 8738 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
There was a problem hiding this comment.
@BenHenning, I took a higlevel look at the approach, and would just like two clarifications:
- It looks like we are planning to have the
TestPlatformParameterConfigRetrieverreplace the annotation system introduced in Fixes part of #4302 - Introducing a JUnit annotation & rule for overriding Platform Parameters and Feature Flags #5565, since they perform the same ovveride function. - In app module tests like
OnboardingFragmentTest,TestPlatformParameterConfigRetriever.setFlagOverride()is used without first injectingPlatformParameterTestInitializerlike inEventBundleCreatorTestfor example. Is this by design or general work still needing to be updated?
| @@ -389,7 +388,7 @@ class OnboardingProfileTypeFragmentTest { | |||
| ExplorationStorageModule::class, NetworkModule::class, NetworkConfigProdModule::class, | |||
| NetworkConnectionUtilDebugModule::class, NetworkConnectionDebugUtilModule::class, | |||
| AssetModule::class, LocaleProdModule::class, ActivityRecreatorTestModule::class, | |||
| PlatformParameterSingletonModule::class, | |||
| PlatformParameterTestModule::class, | |||
There was a problem hiding this comment.
Nit: Duplicate entry.
| @@ -623,7 +627,7 @@ class AudioLanguageFragmentTest { | |||
| ExplorationStorageModule::class, NetworkModule::class, HintsAndSolutionProdModule::class, | |||
| NetworkConnectionUtilDebugModule::class, NetworkConnectionDebugUtilModule::class, | |||
| AssetModule::class, LocaleProdModule::class, ActivityRecreatorTestModule::class, | |||
| NetworkConfigProdModule::class, PlatformParameterSingletonModule::class, | |||
| NetworkConfigProdModule::class, PlatformParameterTestModule::class, | |||
There was a problem hiding this comment.
Nit: duplicate entry.
| PlatformParameterTestModule::class, | ||
| RobolectricModule::class, PlatformParameterTestModule::class, |
There was a problem hiding this comment.
Nit: duplicate entry.
| @@ -2877,7 +2876,7 @@ class ExplorationActivityTest { | |||
| @Component( | |||
| modules = [ | |||
| RobolectricModule::class, | |||
| TestPlatformParameterModule::class, PlatformParameterSingletonModule::class, | |||
| PlatformParameterTestModule::class, PlatformParameterTestModule::class, | |||
There was a problem hiding this comment.
Nit: duplicate entry.
| @@ -6178,7 +6205,7 @@ class StateFragmentTest { | |||
| ExplorationStorageModule::class, NetworkConnectionUtilDebugModule::class, | |||
| NetworkConnectionDebugUtilModule::class, NetworkModule::class, NetworkConfigProdModule::class, | |||
| AssetModule::class, LocaleProdModule::class, ActivityRecreatorTestModule::class, | |||
| PlatformParameterSingletonModule::class, | |||
| PlatformParameterTestModule::class, | |||
There was a problem hiding this comment.
Nit: duplicate entry.
| @@ -1134,7 +1135,7 @@ class OnboardingFragmentTest { | |||
| @Component( | |||
| modules = [ | |||
| RobolectricModule::class, | |||
| TestPlatformParameterModule::class, PlatformParameterSingletonModule::class, | |||
| PlatformParameterTestModule::class, PlatformParameterTestModule::class, | |||
Explanation
TODO: Finish
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: