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
Filter out app starts with more than 60s #2127
Conversation
Oh cocoa the cutoff was 5 min. Is one value more likely to be legit than the other? |
No, Also 60s getsentry/sentry-cocoa#1899 |
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.
Only some nits, LGTM!
sentry-android-core/src/main/java/io/sentry/android/core/AppStartState.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AppStartState.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AppStartState.java
Outdated
Show resolved
Hide resolved
…artState.java Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
…artState.java Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
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 🚀
sentry-android-core/src/test/java/io/sentry/android/core/AppStartStateTest.kt
Outdated
Show resolved
Hide resolved
@@ -77,4 +77,16 @@ class AppStartStateTest { | |||
|
|||
assertEquals(400, sut.appStartInterval) | |||
} | |||
|
|||
@Test | |||
fun `getAppStartInterval returns null if more than 60s`() { |
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.
m
: We could add another test with the max app start duration of 59999 ms to validate it is working correctly.
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 don't think this is unnecessary, we already test above and lower the boundary.
The comparison is a simple >=
with a long value rather than some more complicated calculation.
I can do it but sounds like not needed, your call.
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.
If someone changes the MAX_APP_START_MILLIS to 10000000, no test will fail. Up to you.
sentry-android-core/src/main/java/io/sentry/android/core/AppStartState.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #2127 +/- ##
=========================================
Coverage 80.93% 80.93%
Complexity 3254 3254
=========================================
Files 231 231
Lines 11951 11951
Branches 1586 1586
=========================================
Hits 9673 9673
Misses 1698 1698
Partials 580 580 Continue to review full report at Codecov.
|
sentry-android-core/src/main/java/io/sentry/android/core/AppStartState.java
Outdated
Show resolved
Hide resolved
…artState.java Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
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 🚀
📜 Description
Filter out app starts with more than 60s
💡 Motivation and Context
getsentry/sentry-react-native#2295
💚 How did you test it?
📝 Checklist
🔮 Next steps