Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Technical Analytics: Milestone 2 - Add Ability To Log Feature Flags #5240

Open
wants to merge 115 commits into
base: develop
Choose a base branch
from

Conversation

kkmurerwa
Copy link
Collaborator

@kkmurerwa kkmurerwa commented Nov 22, 2023

Explanation

When merged, this PR will:

  • Create a FeatureFlagLogger.kt file that aggregates and logs all feature flags.
  • Add a FeatureFlagLoggerTest.kt to test the FeatureFlagLogger.
  • Add logging logic to the ApplicationLifecycleObserver.kt file so that it is logged when the app is in foreground.

Screenshot of the FeatureFlagContext log in the Event Logs page

Screenshot of the Event Logs page showing the FeatureFlagsEventContext being logged

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

kkmurerwa and others added 30 commits October 18, 2023 10:01
Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
…a/oppia-android into technical-analytics-milestone-1
…a/oppia-android into technical-analytics-milestone-1
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @kkmurerwa! Just a few follow-ups--PTAL.

@BenHenning BenHenning assigned kkmurerwa and unassigned BenHenning May 7, 2024
Copy link

oppiabot bot commented May 25, 2024

Hi @kkmurerwa, 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.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 25, 2024
@kkmurerwa kkmurerwa removed their assignment May 26, 2024
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 26, 2024
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @kkmurerwa! It seems all that's left here is for the logging identifier controller test to be finalized once #5409 is merged. Please assign back once that's done.

@BenHenning BenHenning assigned kkmurerwa and unassigned BenHenning May 28, 2024
@kkmurerwa kkmurerwa assigned BenHenning and unassigned kkmurerwa May 30, 2024
@kkmurerwa
Copy link
Collaborator Author

@BenHenning PTAL at this PR. The tests are okay now.

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @kkmurerwa! The new test looks good, I just had one suggestion for reformatting. PTAL.

Comment on lines 352 to 365
val appSessionIdProvider = loggingIdentifierController.getAppSessionId()

val appSessionId = monitorFactory.waitForNextSuccessfulResult(appSessionIdProvider)
assertThat(appSessionId).isEqualTo("2a11efe0-70f8-3a40-8d94-4fc3a2bd4f14")

// Simulate a second app open.
TestLoggingIdentifierModule.applicationIdSeed = SECOND_APP_OPEN_APPLICATION_ID
setUpNewTestApplicationComponent()

val appSessionIdProvider2 = loggingIdentifierController.getAppSessionId()

// The app session ID should be different on the second app open.
val appSessionId2 = monitorFactory.waitForNextSuccessfulResult(appSessionIdProvider2)
assertThat(appSessionId2).isEqualTo("c9d50545-33dc-3231-a1db-6a2672498c74")
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
val appSessionIdProvider = loggingIdentifierController.getAppSessionId()
val appSessionId = monitorFactory.waitForNextSuccessfulResult(appSessionIdProvider)
assertThat(appSessionId).isEqualTo("2a11efe0-70f8-3a40-8d94-4fc3a2bd4f14")
// Simulate a second app open.
TestLoggingIdentifierModule.applicationIdSeed = SECOND_APP_OPEN_APPLICATION_ID
setUpNewTestApplicationComponent()
val appSessionIdProvider2 = loggingIdentifierController.getAppSessionId()
// The app session ID should be different on the second app open.
val appSessionId2 = monitorFactory.waitForNextSuccessfulResult(appSessionIdProvider2)
assertThat(appSessionId2).isEqualTo("c9d50545-33dc-3231-a1db-6a2672498c74")
monitorFactory.ensureDataProviderExecutes(loggingIdentifierController.getAppSessionId())
// Simulate a second app open.
TestLoggingIdentifierModule.applicationIdSeed = SECOND_APP_OPEN_APPLICATION_ID
setUpNewTestApplicationComponent()
// The app session ID should be different on the second app open.
val appSessionIdProvider = loggingIdentifierController.getAppSessionId()
val appSessionId = monitorFactory.waitForNextSuccessfulResult(appSessionIdProvider)
assertThat(appSessionId).isEqualTo("c9d50545-33dc-3231-a1db-6a2672498c74")

Couple things:

  1. It's generally preferred to have just a single assert step (since otherwise it means the test is verifying more than one behavior, and we want to keep tests focused on one behavior at a time). We can rely on the earlier tests to verify that the "before" state is correct.
  2. This rearrangement intends to make the 'arrange/act/assert' blocks of the test a bit clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BenHenning Thanks for this. I have updated the test to have a better flow.

@BenHenning
Copy link
Sponsor Member

Also @kkmurerwa please update the PR with the latest develop.

@BenHenning BenHenning assigned kkmurerwa and unassigned BenHenning Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants