Fixes part of #4302 - Introducing a JUnit annotation & rule for overriding Platform Parameters and Feature Flags #5565
Conversation
This file contains 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
…ameters Initial commit, this works with individual overrides while having multiple enable / diasable annotations mess with the overriding and cause the default value to be thrown even with a dummy feature flag name and this commit doesn't address platform parameter value overrides and doesnot check for Oppia Test Rule to be present on all tests.
…or both feature flags and parameter value overrides
|
@BenHenning @adhiamboperes Would it be possible to provide a draft review or approval on the implementation to ensure things are on the right track before I proceed with the rest? I would like to confirm if the mentioned
is done right. If not then will rework based on the feedback. |
|
Reviewed during CLaM meeting today and feedback was left in real-time. |
Everything looks right except the overriding mechanism. Rather than having an override built into the production module, we should be overriding the value via OkHttp and the platform parameter syncing. This might be a bit tricky to set up, so it may well be worth getting this working before migrating all uses over to the new system. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi @Rd4dev, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
…o annotations_test_rules_platform_parameter
oppiabot
bot
removed
the
stale
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi @Rd4dev, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Rd4dev
removed
the
stale
This comment was marked as outdated.
This comment was marked as outdated.
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), 29 KiB (Removed) APK download size (estimated): 17 MiB (old), 17 MiB (new), 19 KiB (Removed) Method count: 260177 (old), 259603 (new), 574 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6816 (new), 2 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 29 KiB (Removed)
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), 21 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 16 KiB (Removed) Method count: 116253 (old), 116054 (new), 199 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5784 (new), 2 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 21 KiB (Removed)
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), 21 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 12 KiB (Removed) Method count: 116259 (old), 116077 (new), 182 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5784 (new), 2 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 21 KiB (Removed)
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), 21 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 11 KiB (Removed) Method count: 116259 (old), 116077 (new), 182 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5784 (new), 2 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 21 KiB (Removed)
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) |
Yet to add for override platform parameters.
Coverage ReportResultsNumber of files assessed: 269 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
Unsure of string type parameters and int type parameters were untested.
…TestModule, updated wiki page
|
@BenHenning, @adhiamboperes PTAL, and I also wanted to confirm a few requirements:
|
Coverage ReportResultsNumber of files assessed: 271 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
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), 4768 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 6107 bytes (Added) Method count: 260202 (old), 260227 (new), 25 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 4768 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), 0 bytes (No change) APK download size (estimated): 10 MiB (old), 10 MiB (new), 21 bytes (Removed) Method count: 116281 (old), 116281 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) 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), 1212 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1986 bytes (Added) Method count: 116287 (old), 116310 (new), 23 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 1212 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), 1208 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 3964 bytes (Added) Method count: 116287 (old), 116310 (new), 23 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 1208 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) |
Rd4dev
commented
Jan 16, 2025
| /** | ||
| * Extracts all feature flag annotations of the specified type from a collection of annotations. | ||
| * | ||
| * @param annotations a collection of annotations to be checked for feature flag annotations. |
There was a problem hiding this comment.
Todo: To remove ending periods from all KDocs.
Suggested change
| * @param annotations a collection of annotations to be checked for feature flag annotations. | |
| * @param annotations a collection of annotations to be checked for feature flag annotations |
|
Thanks @Rd4dev! Reviewing this today. |
Thanks @adhiamboperes, appreciate the update! |
BenHenning
reviewed
Jan 21, 2025
There was a problem hiding this comment.
Thanks @Rd4dev! This is really exciting to see progress on.
I only looked at the top-level override classes for now, and still need to dig deeper into the PR. However, I do have one thought that I wanted to run by you.
I've had an idea for some time to leverage code generation for the feature flags to minimize the amount of Dagger binding boilerplate that needs to be done each time a new flag is added. Originally, I thought that this would require a custom annotation processor. However, now that Gradle is gone, we actually could do this with protobuf. Consider:
- A new textproto file that lists every feature flag and platform parameter with server string names and default values. I suspect this would be a twofold definition: explicit parameters in a proto file, and server names and default values in textproto (so that we have strong typing for the separate flags).
- Automatic compilation to binary proto (like we do with the language configuration under
config/). - Some file loading logic similar to that which is needed for the languages configuration.
- Updating Dagger bindings that just generate the entire set of bound flags/parameters by loading the textproto file.*
* Note that this is a little trickier than it sounds because we can't technically do file I/O within Dagger providers without incurring some performance issues. However, something that we could do is automatically bind implementation classes based on the proto structure (which is available at compile time since the individual flags would be defined within a proto file), and then have the implementations support asynchronously loading their state (either from the local file cache or from their local file configuration). It's possible we don't even need this second bit since I think the platform parameter system is technically supposed to be designed to fully load its asynchronous state during the app startup so that all flag values are fixed for the lifetime of the app process.
While a long explanation, I suspect that the delta might be relatively small and also be a pretty substantial improvement to how flags and parameters are interacted with in both tests and production code. There's probably more that could be done with real code generation, but that starts getting a bit scary in scope here.
Is this something you'd want to take on as part of this PR? If not, no worries. I just wanted to ask since you've already spent a lot of time thinking about the mechanisms. :)
| */ | ||
| @Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION) | ||
| @Repeatable | ||
| annotation class EnableFeatureFlag(val name: FeatureFlag) |
There was a problem hiding this comment.
For both this and the disable variant, it would be good to explain here how cascading cases or repeated cases are handled. For instance, consider the following setup:
@EnableFeatureFlag(FEATURE_1)
@DisableFeatureFlag(FEATURE_1)
class MyConfusingTest {
@Test
fun test_case1() {}
@Test
@EnableFeatureFlag(FEATURE_1)
fun test_case2() {}
@Test
@DisableFeatureFlag(FEATURE_1)
fun test_case3() {}
@Test
@EnableFeatureFlag(FEATURE_1)
@DisableFeatureFlag(FEATURE_1)
fun test_case4() {}
@Test
@DisableFeatureFlag(FEATURE_1)
@EnableFeatureFlag(FEATURE_1)
fun test_case5() {}
}What is the expected state of FEATURE_1 in each test case? Is it an error or does latest win?
Personally, I would expect that:
- More than one declaration for a single feature at the class or method level results in an error (i.e.
MyConfusingTest,test_case4, andtest_case5above should all fail). - It's allowed to have at most 2 overrides for a single feature for a given test: one at the class level, and one at the method level, where the method annotation always takes precedence. This allows us to enable/disable a feature for the whole class, then selectively enable/disable it on a per-test basis as needed.
| */ | ||
| @Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION) | ||
| @Repeatable | ||
| annotation class OverrideStringParameter(val name: PlatformParameter, val value: String) |
There was a problem hiding this comment.
For this & the other override classes: see my comment in EnableFeatureFlag as I think the same consideration is needed here on how cascading and overriding scenarios are handled.
| */ | ||
| @Target(AnnotationTarget.FUNCTION) | ||
| @Repeatable | ||
| annotation class ResetFeatureFlagToDefault(val name: FeatureFlag) |
There was a problem hiding this comment.
Similar to my comment in EnableFeatureFlag, cascading/duplication needs to be considered here. I'd expect any cases where an enable/disable happens and reset at the same level to be an error (i.e. a test method can only reset, not enable/disable and then reset).
For strictness, I'd also expect that reset throws an error if there's no explicit override of the feature at the class level (i.e. there's no reason to reset). While resetting a non-overridden flag is a no-op, allowing it to be checked in could result in false assumptions and a fundamental misunderstanding of what the test and surrounding test suite is actually doing.
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.
Explanation
Fixes part of #4302
This PR includes:
Feature Flags Enhancements:
Refactoring Test Module Constants:
Test File Migration:
TestModule.Oppia Test Rule Validation:
@get:Rule val oppiaTestRule = OppiaTestRule()in test files.Wiki Page Updated
https://github.com/oppia/oppia-android/blob/bcc9d4a9a670673a623240876192b44f5ddc750f/wiki/Platform-Parameters-%26-Feature-Flags.md#how-to-write-tests-related-to-platform-parameters
Todo:
Essential Checklist