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.

Sign up for GitHub

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

Jump to bottom

[BUG]: Some tests on the LoggingIdentifierControllerTest file are false positives #5399

Closed
kkmurerwa opened this issue May 18, 2024 · 1 comment · Fixed by #5409
Closed

[BUG]: Some tests on the LoggingIdentifierControllerTest file are false positives #5399

kkmurerwa opened this issue May 18, 2024 · 1 comment · Fixed by #5409
Assignees
@BenHenning
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@kkmurerwa
Copy link
Collaborator

Describe the bug

Tests in the test file that involve restarting the app and checking if the value generated is the same will always pass because of how the test file is set up. The LoggingIdentifierControllerTest initializes with the applicationIdSeed of 1L and on restart, this is the same seed used to generate the IDs and as such, any ID generated with the same seed will always be the same. Tests affected by this issue include the;

  • testGetInstallationId_secondAppOpen_providerReturnsSameInstallationIdValue
  • testFetchInstallationId_secondAppOpen_returnsSameInstallationIdValue
  • testGetSessionId_secondCall_returnsSameRandomId

Two new test also needs to be introduced to ensure that the ID generated for the sessionID and the appSessionId values change when the app is restarted.

Steps To Reproduce

N/A

Expected Behavior

I expect that when the app restart is simulated, the applicationSeedId should be varied or modified on the restart to ensure that generated IDs are not the same purely on the basis of being generated with the same seed ID but because the IDs are being cached by the app itself. In the instance of the appSessionId, the ID should be different on each app restart in the tests to test the expected behaviour of the ID.

Screenshots/Videos

No response

What device/emulator are you using?

No response

Which Android version is your device/emulator running?

No response

Which version of the Oppia Android app are you using?

No response

Additional Context

No response
@kkmurerwa kkmurerwa added bug End user-perceivable behaviors which are not desirable. triage needed labels May 18, 2024
@kkmurerwa kkmurerwa mentioned this issue May 18, 2024
Merged
6 tasks
@BenHenning BenHenning mentioned this issue May 26, 2024
Merged
6 tasks
@BenHenning
Copy link
Member

This was a very good call-out. I sent a PR to fix these tests, and fortunately the controller is behaving as expected (so there were no bugs to address, only incorrect tests + new tests to add).

adhiamboperes added a commit that referenced this issue May 30, 2024
@BenHenning @adhiamboperes
Fix #5399: Fix tests in LoggingIdentifierControllerTest (#5409)
3c7713d 
## Explanation

Fixes #5399

This PR addresses the problems outlined in #5399 by:
- Ensuring that
``testGetInstallationId_secondAppOpen_providerReturnsSameInstallationIdValue``
and
``testFetchInstallationId_secondAppOpen_returnsSameInstallationIdValue``
are correctly run in a _new_ ``TestApplication`` (as inspired by
``SplashActivityTest``).
- Adding a new test to verify that session ID correctly regenerates in a
new app instance (``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.
- Improved the tests around cache corruption (via emptying) and deletion
to ensure they correctly demonstrate the failure fallbacks (empty/null
for corruption and reinitialization for deletion in the same way as a
new app install).

## Essential Checklist
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
N/A -- This is fixing a specific test and has no impact on UI behaviors.

---------

Co-authored-by: Adhiambo Peres <[email protected]>
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks. labels May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BenHenning BenHenning

Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
Projects
[Team] Developer Workflow & Infrastructure - Android
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #5399: Fix tests in LoggingIdentifierControllerTest oppia/oppia-android
4 participants
@seanlip @BenHenning @kkmurerwa @adhiamboperes