-
Notifications
You must be signed in to change notification settings - Fork 521
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
Fix #5399: Fix tests in LoggingIdentifierControllerTest #5409
Fix #5399: Fix tests in LoggingIdentifierControllerTest #5409
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed changes.
@adhiamboperes PTAL for codeowners. @kkmurerwa PTAL to verify that this addresses your points in #5399 as expected. I deviated slightly from the suggestions since installation ID should be retained on app restarts unless the database is deleted, whereas session ID should be regenerated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked at the changes and they address the problems I had highlighted in the issue. The only part not included are the appSessionId tests which are understandably not in the codebase yet. I will add them once this PR is merged.
Unassigning @kkmurerwa since they have already approved the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @BenHenning !
Updating with latest develop and enabling auto-merge to unblock @kkmurerwa |
Unassigning @adhiamboperes since they have already approved the PR. |
Thanks much for the review @adhiamboperes! |
Explanation
Fixes #5399
This PR addresses the problems outlined in #5399 by:
testGetInstallationId_secondAppOpen_providerReturnsSameInstallationIdValue
andtestFetchInstallationId_secondAppOpen_returnsSameInstallationIdValue
are correctly run in a newTestApplication
(as inspired bySplashActivityTest
).testGetSessionId_secondAppOpen_returnsNewRandomId
) which also has the added benefit of providing confidence that the earlier tests are correctly verifying that the installation ID isn't changing, as expected, on a new app instance.Essential Checklist
For UI-specific PRs only
N/A -- This is fixing a specific test and has no impact on UI behaviors.