Extract `EngagementManager` and cover it with unit tests #82

wzieba · 2023-10-24T14:08:10Z

Description

This PR extracts EngagementManager to a separate unit and adds unit tests for it.

The proposed unit test is based on manual Thread#sleep calls and is time-sensitive. It's due to usage of java.utils.Timer which runs a new thread internally - a thread we cannot control. Still, the unit test is written in a way to not be flaky (usages of org.assertj.core.api.isCloseTo assertions) and brings value. E.g., the current test suite covers fix added in #62 .

How to test

Review and green light on CI should be just fine.

So this method can be accessed by extracted `EngagementManager`

The logic itself is complex and should be unit tested. Also, the production implementation will return long intervals (like 10s). Because `EngagementManager` uses TimerTask we have to use `Thread#sleep` in unit tests. All in all - with default implementation of `updateLatestInterval` the unit tests would last very long, hence this extraction, which will allow to inject custom intervals.

To improve readability in tests.

To validate that the tracked event is correct.

The test will contain many unchecked casts, but they're safe - they'll always be <String, Any>.

As per comment: `inc` will always be 0 because the interval between events is lower than 1s

codecov · 2023-10-24T14:11:42Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (b2882f5) 32.95% compared to head (98dfeaa) 42.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   32.95%   42.06%   +9.10%     
==========================================
  Files           5        7       +2     
  Lines         355      359       +4     
  Branches       56       56              
==========================================
+ Hits          117      151      +34     
+ Misses        225      194      -31     
- Partials       13       14       +1

Files	Coverage Δ
...ava/com/parsely/parselyandroid/ParselyTracker.java	`11.41% <20.00%> (+2.82%)`	⬆️
...elyandroid/UpdateEngagementIntervalCalculator.java	`14.28% <14.28%> (ø)`
.../com/parsely/parselyandroid/EngagementManager.java	`69.56% <69.56%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wzieba · 2023-10-25T10:01:25Z

parsely/src/main/java/com/parsely/parselyandroid/UpdateEngagementIntervalCalculator.java

+    private static final long OFFSET_MATCHING_BASE_INTERVAL = 35;
+    private static final double BACKOFF_PROPORTION = 0.3;
+
+    long updateLatestInterval(@NonNull final Calendar startTime) {


Unit tests for this class are planned to be added in a separate PR.

ParaskP7

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, everything works as expected, another great testing job well done! 🌟 🧪 🌟

But wow, that is a very convoluted test, I am very sorry you had to go through all this... 🫂

I have left a warning (⚠️). a couple of questions (❓), a few suggestions (💡) and some minor (🔍) comments for you to consider. I am going to NOT going to approve this PR for now, just because of this one warning (⚠️) comment, but other than that everything seems okay to me.

EXTRAS

Suggestion (💡): I have noticed that during you refactor commits some of them wouldn't be able to build, just because it would require this other subsequent commit that you add later on. I would personally merge those 2 commits together and make it easier for the reviewer to understand that in order to (for example) extract a class, one would also need to make some methods more public (package private). 🤔
Suggestion (💡): In this PR, I am also seeing some additional commits that are not part of this PR. Consider rebasing you branch with the main one before open a PR for review. This will make it easier for reviewers to know exactly what need to be reviewed and avoid having them to figure out the out-of-scope commits.

ParaskP7 · 2023-10-26T09:19:05Z

parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.java

+    public Map<String, Object> baseEvent;
+    private boolean started;
+    private final Timer parentTimer;
+    private TimerTask waitingTimerTask;


Suggestion (💡): I suggest making this @Nullable and then checking that during stop():

public void stop() { if (waitingTimerTask != null) { waitingTimerTask.cancel(); } started = false; }

PS: Actually, I would even recommend for such refactor/test PRs to add any missing nullability annotation wherever possible going forward. 🤷

parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.java

ParaskP7 · 2023-10-26T09:22:46Z

parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.java

+        started = false;
+    }
+
+    public boolean isSameVideo(String url, String urlRef, ParselyVideoMetadata metadata) {


Suggestion (💡): Consider resolving/suppressing all the warnings related to this method:

Suppress: Unchecked cast: 'java.lang.Object' to 'java.util.Map<java.lang.String,java.lang.Object>'

Resolve: Method invocation 'equals' may produce 'NullPointerException'

Resolve: Unboxing of 'baseMetadata.get("duration")' may produce 'NullPointerException'

FYI: For no.2 there is an easy fix, just flit the equals(...), for example:

BEFORE: baseEvent.get("url").equals(url)
AFTER: url.equals(baseEvent.get("url")

I'd leave this to a different PR - I wanted this PR to be focused on extracting EngagementManager and adding minimal set of changes to make it testable.

parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.java

parsely/src/main/java/com/parsely/parselyandroid/UpdateEngagementIntervalCalculator.java

parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt

ParaskP7 · 2023-10-26T09:39:12Z

parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt

+    }
+
+    @Test
+    fun `when starting manager, then schedule task each interval period`() {


Question (❓): Please help me understand how start() works on EngagementManager for multiple events. 🙏

FYI: I am asking that because, to my understanding, a new instance of EngagementManager is created with just one event, the baseEvent, and that is the only event needed to be enqueued. I am struggling a bit to understand the logic here and when we can have multiple of the same events queuing. 🤔

PS: This is related to 51a4674 .

When we call start on EngagementManager, the java.utils.TimerTask is scheduled inside java.utils.Timer. The next execution of the TimerTask will schedule a subsequent task.

So this test checks if subsequent tasks are executed and if they have correct parameters like e.g. updated total timestamp. Does this answer your questions @ParaskP7 ?

Hmmm, yes, I understand how the TimerTask will schedule a subsequent task. I just don't understand when multiple events, mostly in production, could be scheduled, can you help me with that? 🙏

engagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, event, intervalCalculator); engagementManager.start();

I see the above happening on production, which on (for example) startEngagement(...) creates a new instance of EngagementManager with a specific single event. How is this manager is able to handle multiple events, when does this happens, this is what I don't understand @wzieba . 🤔

How is this manager is able to handle multiple events, when does this happens, this is what I don't understand @wzieba . 🤔

When you say "multiple events" do you mean "multiple engagement sessions"? If yes, then the SDK cannot handle it.

Circling back to the original question:

I am asking that because, to my understanding, a new instance of EngagementManager is created with just one event, the baseEvent, and that is the only event needed to be enqueued.

Yes, the EngegementManager is created with just one event, the baseEvent. But no - it's not the only event needed to be enqueued. After the initial baseEvent is enqueued, enqueuing of the next event is scheduled.

When interval time passes, the SDK, based on baseEvent, enqueues a new event with modifications done in doEnqueue. It modifies, e.g. ts, inc or tt parameters.

When you say "multiple events" do you mean "multiple engagement sessions"? If yes, then the SDK cannot handle it.

Good to know! 💯

Yes, the EngegementManager is created with just one event, the baseEvent. But no - it's not the only event needed to be enqueued. After the initial baseEvent is enqueued, enqueuing of the next event is scheduled.

👍

When interval time passes, the SDK, based on baseEvent, enqueues a new event with modifications done in doEnqueue. It modifies, e.g. ts, inc or tt parameters.

That explains it, and apologies for only knowing the basics of the SDK, trying to figure out the rest during reviews... 😊

So, when startEngagement(...) gets triggered, it is always triggered with a baseEvent, and then, until stopEngagement(...) is called, the same baseEvent will be enqueued multiple time, based on the interval, with only some of its parameters being modified. If so, me repeating correctly what you already said, I then think I got the complete picture now, thanks so much for explaining. 🙇

the same baseEvent will be enqueued multiple time, based on the interval, with only some of its parameters being modified

Yes, exactly this 🙌 !

ParaskP7 · 2023-10-26T09:42:18Z

parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt

+        withTotalTime: AbstractLongAssert<*>.() -> AbstractLongAssert<*>,
+        withTimestamp: AbstractLongAssert<*>.() -> AbstractLongAssert<*>,
+    ): MapAssert<String, Any> {
+        return containsEntry("action", "heartbeat")


Minor (🔍): Consider moving the .hasEntrySatisfying("data") check above the .containsEntry("inc", 0L) check to make it easier to follow the same logical order found within the EngagementManager.doEnqueue(...) method, that is, without having to jump up and down to verify test correctness.

return containsEntry("action", "heartbeat") // Incremental will be always 0 because the interval is lower than 1s .hasEntrySatisfying("data") { data -> data as Map<String, Any> assertThat(data).hasEntrySatisfying("ts") { timestamp -> timestamp as Long assertThat(timestamp).withTimestamp() }.containsAllEntriesOf(testData.minus("ts")) }.containsEntry("inc", 0L) .hasEntrySatisfying("tt") { totalTime -> totalTime as Long assertThat(totalTime).withTotalTime() }

parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt

ParaskP7 · 2023-10-26T09:57:01Z

parsely/src/main/java/com/parsely/parselyandroid/UpdateEngagementIntervalCalculator.java

+import java.util.Calendar;
+import java.util.TimeZone;
+
+class UpdateEngagementIntervalCalculator {


Suggestion (💡): Ah, I forgot to mention this in my review, what about making this a Utils class instead of instantiating it as an object. Maybe even a Kotlin utils class, wdyt? 🤔

In next iterations, I'll inject some Clock to this class to fake the current time. Otherwise, test for this class will be time-sensitive. So I'll have to leave this as a class, not a static one.

Nice to know, thanks for the heads-up on that. 💯

For better readability.

ParaskP7

Took it for another spin, everything LGTM now @wzieba , thanks again! 🚀

wzieba · 2023-10-26T11:04:54Z

Thank you @ParaskP7 ! 🎉

wzieba · 2023-10-27T07:37:24Z

@ParaskP7 one question on the initial review:

Suggestion (💡): In this PR, I am also seeing some additional commits that are not part of this PR. Consider rebasing you branch with the main one before open a PR for review.

Which additional commits did you have in mind?

ParaskP7 · 2023-10-27T08:16:37Z

👋 @wzieba and thanks for the reply! 🙇

Those two below got me a bit confused:

🤷

I remembered reviewing those two on this #81 PR, see bottom of this commit list:

🤷

Does that help? 🙏

wzieba · 2023-10-27T10:07:38Z

I see, thanks! It's interesting though that those commits have different hashes. Normally, we shouldn't see those commits after merging main.

ParaskP7 · 2023-10-27T10:10:28Z

Yeaaa, not sure what happened there, that's why, by default, I always recommend rebasing branches with latest main branch before opening a PR... 🤷

wzieba added 14 commits October 20, 2023 14:18

refactor: extract EngagementManager

0327b22

feat: make enqueueEvent package-visible

a2aff95

So this method can be accessed by extracted `EngagementManager`

refactor: use getters instead of field values

7a2c289

tests: record first event after given interval

becb67f

refactor: update tracker and manager to use interval calculator

7767307

tests: record event after each interval

cf6366f

tests: introduce typealias Event

b0dd745

To improve readability in tests.

tests: improve "start manager" test case

f66493c

To validate that the tracked event is correct.

style: suppress UNCHECKED_CAST on EngagementManagerTest

97b35ad

The test will contain many unchecked casts, but they're safe - they'll always be <String, Any>.

style: extract event assertions

7fbc013

tests: narrow down timestamp condition in single event record case

3f6ead9

tests: simplify single event test case

534f5b2

As per comment: `inc` will always be 0 because the interval between events is lower than 1s

tests: add test case for validity of subsequent events

51a4674

wzieba added 4 commits October 25, 2023 11:55

style: move tearDown below setUp

1d2b94c

refactor: move JSON pixel payload to a file

cb4d8a7

tests: extract data to variable

e4dd447

style: move helper method below test cases

b9f8797

wzieba force-pushed the engagement_manager_tests branch from 4c8c002 to b9f8797 Compare October 25, 2023 09:57

Merge branch 'main' into engagement_manager_tests

bf459f5

wzieba commented Oct 25, 2023

View reviewed changes

wzieba marked this pull request as ready for review October 25, 2023 10:02

wzieba requested a review from ParaskP7 October 25, 2023 10:02

ParaskP7 requested changes Oct 26, 2023

View reviewed changes

ParaskP7 reviewed Oct 26, 2023

View reviewed changes

wzieba added 4 commits October 26, 2023 12:06

style: move assertions to single-line

d752495

For better readability.

fix: remove unwanted log from interval calculator

9855f16

tests: decrease the timestamp threshold on single-event case

e67e603

style: move unchecked cast suppress closer to warning

e6a556d

wzieba mentioned this pull request Oct 26, 2023

Use Android logger instead of system's output stream #83

Open

wzieba added 3 commits October 26, 2023 12:48

style: move private methods closer in test

227f9a5

style: static code readability improvements

654e374

style: remove redundant casting of baseEventData

98dfeaa

ParaskP7 approved these changes Oct 26, 2023

View reviewed changes

wzieba merged commit 1caf06f into main Oct 26, 2023
3 checks passed

wzieba deleted the engagement_manager_tests branch October 27, 2023 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract `EngagementManager` and cover it with unit tests #82

Extract `EngagementManager` and cover it with unit tests #82

wzieba commented Oct 24, 2023 •

edited

codecov bot commented Oct 24, 2023 •

edited

wzieba Oct 25, 2023 •

edited

ParaskP7 left a comment

ParaskP7 Oct 26, 2023

ParaskP7 Oct 26, 2023 •

edited

wzieba Oct 26, 2023

ParaskP7 Oct 26, 2023

wzieba Oct 26, 2023

ParaskP7 Oct 26, 2023

wzieba Oct 26, 2023

ParaskP7 Oct 26, 2023

wzieba Oct 26, 2023

ParaskP7 Oct 26, 2023

ParaskP7 Oct 26, 2023

wzieba Oct 26, 2023

ParaskP7 Oct 26, 2023

ParaskP7 left a comment

wzieba commented Oct 26, 2023

wzieba commented Oct 27, 2023

ParaskP7 commented Oct 27, 2023

wzieba commented Oct 27, 2023

ParaskP7 commented Oct 27, 2023

Extract EngagementManager and cover it with unit tests #82

Extract EngagementManager and cover it with unit tests #82

Conversation

wzieba commented Oct 24, 2023 • edited

Description

How to test

codecov bot commented Oct 24, 2023 • edited

Codecov Report

wzieba Oct 25, 2023 • edited

Choose a reason for hiding this comment

ParaskP7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParaskP7 Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParaskP7 left a comment

Choose a reason for hiding this comment

wzieba commented Oct 26, 2023

wzieba commented Oct 27, 2023

ParaskP7 commented Oct 27, 2023

wzieba commented Oct 27, 2023

ParaskP7 commented Oct 27, 2023

Extract `EngagementManager` and cover it with unit tests #82

Extract `EngagementManager` and cover it with unit tests #82

wzieba commented Oct 24, 2023 •

edited

codecov bot commented Oct 24, 2023 •

edited

wzieba Oct 25, 2023 •

edited

ParaskP7 Oct 26, 2023 •

edited