tests for Android profiling #1949
Conversation
fixed few issues found via tests
stefanosiano
requested review from
marandaneto,
romtsn and
maciejwalkowiak
as code owners
|
I still need to add tests for |
Codecov Report
@@ Coverage Diff @@
## feat/profiling/android #1949 +/- ##
=========================================================
Coverage ? 75.21%
Complexity ? 2279
=========================================================
Files ? 228
Lines ? 8167
Branches ? 873
=========================================================
Hits ? 6143
Misses ? 1605
Partials ? 419 Continue to review full report at Codecov.
|
changed device_cpu_frequencies from List<String> -> List<Integer>
changed device_cpu_frequencies from List<String> -> List<Integer> added changelog
| @@ -41,7 +43,7 @@ | |||
| private @Nullable File traceFile = null; | |||
| private @Nullable File traceFilesDir = null; | |||
| private @Nullable Future<?> scheduledFinish = null; | |||
| private volatile @Nullable ITransaction activeTransaction = null; | |||
| @VisibleForTesting volatile @Nullable ITransaction activeTransaction = null; | |||
There was a problem hiding this comment.
Can we test the class without VisibleForTesting? I see this almost in every class.
There was a problem hiding this comment.
Avoiding it would make it harder/less readable to test the code without increasing the visibility of the field.
I realized that when using org.jetbrains.annotations.VisibleForTesting Android studio doesn't care whether you use it in tests or outside them, and builds in any case.
However, using androidx.annotation.VisibleForTesting makes Android studio complain if you access that field outside tests.
Of course, that would work in sentry-android-* only. Would it make any difference?
There was a problem hiding this comment.
The problem is that we could not use androidx.annotation in non-Android projects and that would make 2 different packages of annotations across the project, so we preferred to keep the standard.
You can configure AS to warn in such cases well.
You can also test the behavior of the method without using VisibleForTesting, for example, when you finish the transaction 2 times, you can verify that the captureTransaction method was called only once, so the activeTransaction was actually null during the 2nd time, instead of asserting the field directly.
Usually VisibleForTesting is only necessary when you are working with static fields or when its technically not possible to test the behaviour, it does not like this use case for this field.
Odd that codecov didn't work |
Replaced all over the project: org.junit.Ignore -> kotlin.test.Ignore org.junit.Before -> kotlin.test.BeforeTest org.junit.After -> kotlin.test.AfterTest
📜 Description
We added profiling to the android sdk. Now let's add tests!
💚 How did you test it?
Unit tests on everything touched by last pr
📝 Checklist