Add unit tests to `ParselyAPIConnection` and minor refactor of it #81

wzieba · 2023-10-20T10:28:13Z

Description

This PR adds unit tests for ParselyAPIConnection and adds some minor refactor to it.

How to test

Review and running unit tests should be just fine. Due to async nature of, well, AsyncTask, you can also verify that each test execution waited for AsyncTask to finish. You should see Pixel request success or Pixel request exception at the end of test logs.

Just above we clear `eventQueue` and purge locally stored events. There's no need to check the size of the queues.

This way, we don't have to check for nullability of the tracker. Also using `ParselyAPIConnection` without first initialising `ParselyTracker` doesn't seem to make any use case, so I think it's a save change.

BREAKING CHANGE: `ParselyAPIConnection` is no longer accessible for library consumers. This is a deliberate design change. The `ParselyAPIConnection` on its own doesn't provide value for library consumers: it doesn't do anything domain-specific for Parsely, except purging queue after successful events. I think it's completely save to make it accessible only from the library.

No need for this field to be `public`

Every time SDK purges in-memory events queue, it purges stored queue and vice-versa. The extraction of method asserts this behaviour and increases readability

To unit test HTTP communication logic, framework agnostic.

Previously, the `ParselyAPIConnection` task was not finishing execution before the test finished. Only because the task's `onBackground` was fast, the test was passing. To remove this flakiness, the introduced change uses LooperMode.Mode.PAUSED to wait for AsyncTask execution to finish, before running assertions.

See ab4fe3a for details.

…t is made

…onse

The `object` state persisted between tests, what influenced results depending on order of tests execution.

There's no need to return `HttpURLConnection`. This change simplifies implementation.

codecov · 2023-10-20T10:29:12Z

Codecov Report

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

Comparison is base (22a8241) 20.44% compared to head (0f21123) 32.95%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #81       +/-   ##
===========================================
+ Coverage   20.44%   32.95%   +12.51%     
===========================================
  Files           5        5               
  Lines         362      355        -7     
  Branches       58       56        -2     
===========================================
+ Hits           74      117       +43     
+ Misses        278      225       -53     
- Partials       10       13        +3

Files	Coverage Δ
...m/parsely/parselyandroid/ParselyAPIConnection.java	`96.00% <100.00%> (+96.00%)`	⬆️
...ava/com/parsely/parselyandroid/ParselyTracker.java	`8.58% <0.00%> (+7.72%)`	⬆️

... and 1 file with indirect coverage changes

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

wzieba · 2023-10-20T10:36:38Z

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

+    }
+
+    companion object {
+        val pixelPayload = """


The auto-format doesn't work here :/ I didn't format this by hand because it looks ok (readable) and I'm concerned about future edits of this field (and a need to format again by hand).

Suggestion (💡): I actually wanted to suggest using test/resources/pixel_payload.json instead. Wdyt? 🤔

wzieba · 2023-10-20T10:38:03Z

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

+    private class FakeTracker : ParselyTracker(
+        "siteId", 10, ApplicationProvider.getApplicationContext()


Initially, I wanted to introduce a package private interface (QueueManager?) but due to limitations of Java (interface methods must be public) I couldn't do it. Hence, the fake class extends the whole ParselyTracker.

Praise (❤️): Actually, I like what you did here, so 👍 !

ParaskP7

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, everything works as expected, great job on yet another test added! 🌟 🧪 🌟

I have left few a couple of suggestions (💡) 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/main/java/com/parsely/parselyandroid/ParselyAPIConnection.java

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

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

ParaskP7 · 2023-10-23T10:59:19Z

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

+    }
+
+    companion object {
+        val pixelPayload = """


Suggestion (💡): I actually wanted to suggest using test/resources/pixel_payload.json instead. Wdyt? 🤔

ParaskP7 · 2023-10-23T11:00:04Z

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

+    private class FakeTracker : ParselyTracker(
+        "siteId", 10, ApplicationProvider.getApplicationContext()


Praise (❤️): Actually, I like what you did here, so 👍 !

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

wzieba · 2023-10-23T11:49:43Z

Thanks @ParaskP7 for the review! I've addressed your suggestions 👍

ParaskP7 · 2023-10-23T12:59:15Z

Awesome, thanks @wzieba , everything LGTM! 🙇 ❤️ 🚀

wzieba added 22 commits October 19, 2023 10:22

refactor: remove unnecessary if check

32e3101

Just above we clear `eventQueue` and purge locally stored events. There's no need to check the size of the queues.

refactor: pass instance of tracker to APIConnection

1427b85

This way, we don't have to check for nullability of the tracker. Also using `ParselyAPIConnection` without first initialising `ParselyTracker` doesn't seem to make any use case, so I think it's a save change.

style: make ParselyAPIConnection#exception private

2a2ad97

No need for this field to be `public`

refactor: extract purgeEventsQueue method

013cf67

Every time SDK purges in-memory events queue, it purges stored queue and vice-versa. The extraction of method asserts this behaviour and increases readability

style: update modifier for purgeStoredQueue

c9825f1

build: add MockWebServer dependency

acbaf7a

To unit test HTTP communication logic, framework agnostic.

tests: introduce FakeTracker

41e0d25

tests: request without pixels case

d1e2c85

tests: request with pixels case

a1b9bd8

Merge branch 'main' into api_connection_unit_tests

bae3ca3

tests: update POST request test to wait for task to finish

85b9489

See ab4fe3a for details.

style: move url declaration to "given"

375237b

tests: add test for purging events queue when successful request is made

803198a

tests: add test for not purging events queue when unsuccessful reques…

6ebc6b0

…t is made

style: make url definition test-wide

a8f3861

tests: update test names to add missing given condition

877aea1

tests: verify that flush timer is stopped after succesful response

155a3c6

tests: verify that flush timer is not stopped after unsuccessful resp…

b95cde6

…onse

tests: change FakeTracker from object to class

8756b27

The `object` state persisted between tests, what influenced results depending on order of tests execution.

refactor: change result type of ParselyAPIConnection to Void

c8795c0

There's no need to return `HttpURLConnection`. This change simplifies implementation.

wzieba commented Oct 20, 2023

View reviewed changes

wzieba requested a review from ParaskP7 October 20, 2023 10:38

wzieba marked this pull request as ready for review October 20, 2023 10:38

ParaskP7 approved these changes Oct 23, 2023

View reviewed changes

wzieba added 2 commits October 23, 2023 13:30

style: move tearDown below setUp

cf858aa

refactor: move JSON pixel payload to a file

0f21123

wzieba merged commit b2882f5 into main Oct 23, 2023
3 checks passed

wzieba deleted the api_connection_unit_tests branch October 23, 2023 11:49

ParaskP7 mentioned this pull request Oct 27, 2023

Extract EngagementManager and cover it with unit tests #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests to `ParselyAPIConnection` and minor refactor of it #81

Add unit tests to `ParselyAPIConnection` and minor refactor of it #81

wzieba commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 20, 2023 •

edited

Loading

wzieba Oct 20, 2023 •

edited

Loading

ParaskP7 Oct 23, 2023

wzieba Oct 20, 2023 •

edited

Loading

ParaskP7 Oct 23, 2023

ParaskP7 left a comment

ParaskP7 Oct 23, 2023

ParaskP7 Oct 23, 2023

wzieba commented Oct 23, 2023

ParaskP7 commented Oct 23, 2023

		private class FakeTracker : ParselyTracker(
		"siteId", 10, ApplicationProvider.getApplicationContext()

Add unit tests to ParselyAPIConnection and minor refactor of it #81

Add unit tests to ParselyAPIConnection and minor refactor of it #81

Conversation

wzieba commented Oct 20, 2023 • edited Loading

Description

How to test

codecov bot commented Oct 20, 2023 • edited Loading

Codecov Report

wzieba Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

ParaskP7 Oct 23, 2023

Choose a reason for hiding this comment

wzieba Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

ParaskP7 Oct 23, 2023

Choose a reason for hiding this comment

ParaskP7 left a comment

Choose a reason for hiding this comment

ParaskP7 Oct 23, 2023

Choose a reason for hiding this comment

ParaskP7 Oct 23, 2023

Choose a reason for hiding this comment

wzieba commented Oct 23, 2023

ParaskP7 commented Oct 23, 2023

Add unit tests to `ParselyAPIConnection` and minor refactor of it #81

Add unit tests to `ParselyAPIConnection` and minor refactor of it #81

wzieba commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 20, 2023 •

edited

Loading

wzieba Oct 20, 2023 •

edited

Loading

wzieba Oct 20, 2023 •

edited

Loading