RUMM-1765 Fix foreground app state reporting in AppStateListener - part 5 🏁
#696
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
2 tasks
…ion on app foreground state as `.active` is not enough to infer if app is in foreground. It might be also in foreground while the app state is `.inactive`.
ncreated
force-pushed
the
ncreated/RUMM-1765-fix-foreground-app-state-reporting-in-app-state-listener
branch
from
1dacc68 to
7a6548a
Compare
maxep
approved these changes
Dec 22, 2021
|
|
||
| XCTAssertEqual(history2.currentState.date.timeIntervalSince(history1.currentState.date), 1.0) | ||
| // When | ||
| (0..<1).forEach { _ in |
There was a problem hiding this comment.
This will do a single iteration :)
Suggested change
| (0..<1).forEach { _ in | |
| (0..<10).forEach { _ in |
There was a problem hiding this comment.
This one need to be addressed before merging. All good otherwise!
There was a problem hiding this comment.
Ops! Fixed, good catch 💪
| ) | ||
| func testWhenAppTransitionsBetweenForegroundAndBackground_itComputesTotalForegroundDuration() { | ||
| let numberOfForegroundSnapshots: Int = .mockRandom(min: 0, max: 20) | ||
| let numberOfBackgroundSnapshots: Int = .mockRandom(min: numberOfForegroundSnapshots == 0 ? 1 : 0, max: 20) // must include at least one snapshot |
There was a problem hiding this comment.
Why not just
Suggested change
| let numberOfBackgroundSnapshots: Int = .mockRandom(min: numberOfForegroundSnapshots == 0 ? 1 : 0, max: 20) // must include at least one snapshot | |
| let numberOfBackgroundSnapshots: Int = .mockRandom(min: 1, max: 20) // must include at least one snapshot |
There was a problem hiding this comment.
I wanted to also cover scenario where numberOfBackgroundSnapshots is 0. And this couldn't be done if numberOfForegroundSnapshots is 0 too, because we need at least one snapshot for:
initialSnapshot: allSnapshots[0],In general, in this fuzzy test I wanted to cover all possibilities:
- app only in foreground,
- app only in background,
- app in both foreground and background.
Base automatically changed from
ncreated/RUMM-1765-send-app-launch-crashes
to
ncreated/RUMM-1765-app-launch-events-tracking
December 22, 2021 11:40
ncreated
deleted the
ncreated/RUMM-1765-fix-foreground-app-state-reporting-in-app-state-listener
branch
2 tasks
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.
What and why?
📦 This is the final PR closing topic of tracking app launch events (and crashes). While testing it (with new options added to debug UI 👇), I found a bug 🐞 in
AppStateListener, which I'm solving in this PR.How?
Problem was that
AppStateListenerwas only consideringUIApplication.State.activeas "foreground" application state, whereas app lifecycle is more complex than that and it can be also in "foreground" while in.inactivestate:Because
.inactivewas not considered "foreground", any event tracked during application launch (e.g. fromapplication(_:didFinishLaunching:)) was starting "Background" instead of "ApplicationLaunch" view. I fixed this issue by exposingAppStatedirectly fromAppStateListenerand letting other components decide by their own on handling particular states.🏁 Final Testing
With all logic in place, I made last tests, ensuring everything works as expected. Here are some explored scenarios:
Review checklist