Extract QueueManager and local storage related code. Cover with unit tests.
#87
Conversation
To LocalStorageRepository.java
The added test proves, that method `expelStoredEvent` doesn't have any affect on the stored queue. Removing it is safe, as it asserts the previous behavior.
This test is added to assert correctness of serialization after migrating LocalStorageRepository from Java to Kotlin. Two languages might have some differences in how they serialize objects, so this test will assert that older storage files (created by Java) will have compatible content with new version of LocalStorageRepository, written in Kotlin.
| @Test | ||
| fun `when expelling stored event, then assert that it has no effect`() { | ||
| // given | ||
| sut.persistQueue((1..100).map { mapOf("index" to it) }) | ||
|
|
||
| // when | ||
| sut.expelStoredEvent() | ||
|
|
||
| // then | ||
| assertThat(sut.getStoredQueue()).hasSize(100) | ||
| } |
There was a problem hiding this comment.
This test proves that expelStoredEvent has no effect on the stored list. I'm thinking we should remove this logic from the SDK. Internal communication: p1699018266041689-slack-C0533SEJ82H
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 44.38% 51.63% +7.24%
==========================================
Files 8 10 +2
Lines 365 368 +3
Branches 56 55 -1
==========================================
+ Hits 162 190 +28
+ Misses 189 165 -24
+ Partials 14 13 -1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This file was created using the current main version of the SDK, which uses Java for serializing the object. With having this file in our tests, I make sure that the migration from Java to Kotlin doesn't break anything if users already had parsely-events.ser file.
wzieba
force-pushed
the
extract_queuemanager_local_storage
branch
from
8cafd83 to
8e573f0
Compare
There was a problem hiding this comment.
👋 @wzieba !
I have reviewed this PR as per the instructions, everything works as expected, good job, testing rocks! 🌟
I have left few questions (❓), one 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.
EXTRAS
Suggestion (💡): This is just and FYI that your first refactor 8df277c commit was a bit more difficult to review than it could have been, only because of the order of methods had changed during the move.
parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt
Show resolved
Hide resolved
| /** | ||
| * Delete an event from the stored queue. | ||
| */ | ||
| open fun expelStoredEvent() { |
There was a problem hiding this comment.
Question (❓): Now that you have proved (via testing) that this function is actually not doing anything, why not removing this now, rather than keeping it, then keeping this whole if (parselyTracker.storedEventsCount() > STORAGE_SIZE_LIMIT) { ... } logic and also testing against that? 🤔
FYI: I also understand that this is already under discussion (context).
There was a problem hiding this comment.
I completely agree with you: I'm all hands on removing this method: before that, I'll make sure that some fixed capacity of local storage is not a business requirement. I'll revisit this in just next PR.
parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt
Outdated
Show resolved
Hide resolved
parsely/src/test/java/com/parsely/parselyandroid/LocalStorageRepositoryTest.kt
Show resolved
Hide resolved
|
Thanks for the review @ParaskP7 ! I'll proceed with the merge as I think we discussed or addressed every comment. |
|
Great, thanks for the extra commit and answering my comments @wzieba , LGTM! 🙇 🌟 🚀 |
Description
This PR extracts logic of
QueueManagerand extracts local storage related code, encapsulating it toLocalStorageRepositoryclass.It doesn't do anything related to threading model, so it's safe to merge this to
main. After merge, I'll back port it tocoroutinesbranch.