Add setup for functional tests. Test sending events over the local variable limit. #85
Conversation
- add dependencies - add test scaffold - introduce an empty, test-only activity
With address of mocked server as `ROOT_URL`, applied via reflection. `ROOT_URL` *must not* be a plain declaration, because otherwise the compiler will *inline* the value of the constant, and we won't be able to change it, even via reflection, for the functional test.
By using AndroidX test orchestrator with `clearPackageData` flag. Each test should run in homogeneous environment. This change asserts, that the file used to store events locally won't be shared between tests.
This commit adds test that checks if sending events over the limit of 50 events, works as intended. To not use flaky `Thread#sleep` approach, this test checks for: - local storage file changes via `java.nio.file.WatchService` API - finish of HTTP requests via `okhttp3.mockwebserver.takeRequest` API The `waitForFileEvents` method works in a way as `app.cash.turbine`, without a timeout though.
This commit changes the approach for functional tests to more blackbox style, which seems to be more suitable here. As we don't care for implementation details (e.g. how or if events are stored locally), we no longer observe file changes.
of triggered HTTP request.
On a separate job, as it has to run on macOS.
of unit and instrumentation tests
There was a problem hiding this comment.
👋 @wzieba !
I have reviewed and tested this PR as per the instructions, everything works as expected, another great job, more testing, more loving! 🌟 🧪 ❤️
I have left few questions (❓), a couple of suggestions (💡) and some minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt
Show resolved
Hide resolved
| // Waits for the SDK to send events (flush interval passes) | ||
| val requestPayload = server.takeRequest().toMap() | ||
| assertThat(requestPayload["events"]).hasSize(51) | ||
|
|
There was a problem hiding this comment.
Question (❓): Although I understand how this test is working and what it tried to verify, I feel that something is missing here, an assertion of some kind, something that will showcase what was documented on the test case above:
The SDK will save the events to disk......and send them in the next flush interval.At the end, when all events are sent......the SDK will delete the content of local storage file.
I mean, can we assert that the SDK is saving the events first? For example, will it save all 51 of them, or only the 1 event that exceed the threshold, something else? Then, what happens during the first flush interval, what happens during the next flush interval, these kind of question. Apologies for trying to push things, I am just trying to review this as a very naive test reader that would like to understand this SDK through testing, assuming I don't have any prior knowledge or the SDK. Wdyt, too much? 🤔
PS: I also naively tried to add something basic, the assertThat(locallyStoredEvents).isNotEmpty before the waitFor { locallyStoredEvents.isEmpty() }, to see if that will pass, but it failed. 🤷
There was a problem hiding this comment.
I mean, can we assert that the SDK is saving the events first?
This is something I pivoted from in 8d4a0d3. IMO, the functional tests shouldn't check for such implementation details. The things we're interested in/testing here are:
- that SDK has sent the same number of events consumer app tracked (asserted in line 65)
- that SDK doesn't store additional data (line 69). Actually, we shouldn't even check this probably in a functional test, but this is a safe-check as I know we have some issues with events duplications.
Then, what happens during the first flush interval, what happens during the next flush interval, these kind of question.
There is only one flush, and it's tested 👍 I'll be extending this tests suite to cover more cases, like multiple flushes, but here we test "what would happen if user recorded more than 50 events quickly".
PS: I also naively tried to add something basic, the assertThat(locallyStoredEvents).isNotEmpty before the waitFor { locallyStoredEvents.isEmpty() }, to see if that will pass, but it failed. 🤷
Good point, but it's only timing thing. It seems that assertions above take long enough to give SDK time to clean the file.
Try this:
Subject: [PATCH] refactor: move the class to Kotlin
This class is relatively simple and difficult to test because of `java.utils.Timer`. That's why I decided to make migration to Kotlin right away, without unit tests coverage first.
---
Index: parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt
--- a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt (revision 5900d93eff388bc5b2d98c44072cee07d91b1a96)
+++ b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt (date 1698666962291)
@@ -62,11 +62,11 @@
// Waits for the SDK to send events (flush interval passes)
val requestPayload = server.takeRequest().toMap()
+ assertThat(locallyStoredEvents).isNotEmpty
assertThat(requestPayload["events"]).hasSize(51)
// Wait a moment to give SDK time to delete the content of local storage file
waitFor { locallyStoredEvents.isEmpty() }
- assertThat(locallyStoredEvents).isEmpty()
}
}and the test should fail!
WDYT?
There was a problem hiding this comment.
Thanks for the reply @wzieba ! 👍
This is something I pivoted from in 8d4a0d3. IMO, the functional tests shouldn't check for such implementation details. The things we're interested in/testing here are:
- that SDK has sent the same number of events consumer app tracked (asserted in line 65)
- that SDK doesn't store additional data (line 69). Actually, we shouldn't even check this probably in a functional test, but this is a safe-check as I know we have some issues with events duplications.
👍 if you are also 👍 with that, I just wanted to question it a bit... 😊
There is only one flush, and it's tested 👍 I'll be extending this tests suite to cover more cases, like multiple flushes, but here we test "what would happen if user recorded more than 50 events quickly".
Thanks for the clarification on that. 👍
Good point, but it's only timing thing. It seems that assertions above take long enough to give SDK time to clean the file.
Try this:
- assertThat(locallyStoredEvents).isNotEmpty
and the test should fail!
WDYT?
Yea, it is hard to assert those things... 😭
FYI: The test should fail, I don't understand that, the test should success with this change, is it not? It failing was the actual problem I had as well, as I would have expected it to succeed before waiting... 🤔
PS: Apologies, I am a bit confused it seems... 🤷
- assertThat(locallyStoredEvents).isEmpty()
Why did you remove this assertion from the patch you shared? 🤔
There was a problem hiding this comment.
Yes, sorry I made some confusion with the diff! Please ignore the diff in previous comment. But it's still case of timing - check this:
Subject: [PATCH] fix: add `intern` to `ROOT_URL` of emulator localhost
---
Index: parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt
--- a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt (revision d2a25dbc864396d67dc517943a5a079cffb86304)
+++ b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt (date 1698668794333)
@@ -71,6 +71,7 @@
}
private fun RecordedRequest.toMap(): Map<String, List<Event>> {
+ assertThat(locallyStoredEvents).isEmpty()
val listType: TypeReference<Map<String, List<Event>>> =
object : TypeReference<Map<String, List<Event>>>() {}
In this case the test will fail although, in theory, it shouldn't because after making the request, SDK will clean the local file. But, as the things happen on different threads, we don't have certainty what will happen first - assertion, or file deletion.
In case of this test - the assertions and JSON mapping take enough time, that, at least in theory, we shouldn't need to wait. Yet, relying on "JSON serialization being slow" as a determiner of test success is super flaky. Hence, the waitFor method.
There was a problem hiding this comment.
But it's still case of timing...
Thanks for sharing a new diff, and I see, thanks for explaining again! ✅
I don't want to block this PR, feel free to merge it when you think it is ready, I just wanted to question it a bit just to get a better idea on the actual value of this functional test and how you would that be of use later on. 👍
There was a problem hiding this comment.
Thanks for the questions! We can always iterate over how those tests look now. For now, I think this test brings value - we're sure that saving and getting data from the local file is working fine and doesn't produce duplicates.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 44.23% 44.38% +0.15%
==========================================
Files 8 8
Lines 364 365 +1
Branches 56 56
==========================================
+ Hits 161 162 +1
Misses 189 189
Partials 14 14
☔ View full report in Codecov by Sentry. |
Because we only now run instrumentation tests in the `debug` build type by default, we no longer need to configure `debug` as the `testBuildType`.
Description
This PR adds a setup for writing and running functional tests on CI. Contrary to unit tests, they are meant to be written via black-box technique. The only mocked/faked element of the setup is API, which is intentionally mocked via MockWebServer.
Additionally, it adds a test case for tracking number of events exceeding maximum number of events that are stored in memory. This asserts correctness of saving/querying data from the local file as well as shows the benefits of functional tests.
Note
Important pivot how to approach the proposed test was made in 8d4a0d3 so please don't pay a lot of attention to implementation details introduced in commit before, which is e97a7a5.
How to test
Green light on CI should be enough. You can also download the build artifact and see results of instrumentation tests.