Extract building logic to EventsBuilder. Add unit tests for it. #79
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 0.00% 20.44% +20.44%
==========================================
Files 4 5 +1
Lines 358 362 +4
Branches 58 58
==========================================
+ Hits 0 74 +74
+ Misses 358 278 -80
- Partials 0 10 +10
☔ View full report in Codecov by Sentry. |
To improve readability and reduce boilerplate.
They don't provide any new data (not covered lines are still visible on Codecov portal) but they shadow diff change of the whole PR, making it more difficult to read.
There was a problem hiding this comment.
👋 @wzieba !
I have reviewed and tested this PR as per the instructions, everything works as expected, awesome job adding our first unit test! 🌟 🎉 🌟
Comparison is base (e33285b) 0.00% compared to head (d202510) 20.44%.
🎉 🌟 🎉
I have left a question (❓), 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/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| fun `events builder prepares correct pageview pixel`() { |
There was a problem hiding this comment.
DISCLAIMER: This is discussion territory, please take it with a grain of salt since all that is just personal test style, nothing more, nothing less.
- Suggestion (💡): How about always using the
given ..., then ..., when ...pattern for every test description? Actually, I am seeing you already using that patter on a few tests further below, so I am not sure when and why you are choosing one over the other. I would personally prefer such a consistency across every test so I don't have to think when to use which patter. Wdyt? 🤔 - Minor (🔍): When using the
given ..., then ..., when ...pattern, I suggest only thegivenpart be optional, but thewhenandthento be always present. For example, for all of yourgiven ..., then ..., when ...related test below, I notice you are not including thethen, why is that? 🤔 - Minor (🔍): Consider removing the
// given,// whenand// thencomments. IMHO, as long as you split thegiven/whenwith an empty line (when there is agiven), and then thewhen/thenwith another empty line, this is already enough. Otherwise, you get an extra 2/3 lines of code per test, which I debate it being unnecessarily.
There was a problem hiding this comment.
Thanks! I've updated the names of tests, but decided to leave comments. For some reason, they make the test much more readable for me, the space feels easy to misplace by accident. I'll keep thinking about this though :)
I notice you are not including the then, why is that?
Did you mean name of tests? Because every test contain the then part - or am I missing something?
There was a problem hiding this comment.
Thanks! I've updated the names of tests, but decided to leave comments. For some reason, they make the test much more readable for me, the space feels easy to misplace by accident.
Yes, I understand this @wzieba , it is indeed helping if you aren't used to just depend on line breaks.
I'll keep thinking about this though :)
🤔 🥇 😄
Did you mean name of tests? Because every test contain the then part - or am I missing something?
Me being... 🔍 🔍 🔍 😄
An example of such test is this one: given extraData is null, when creating a pixel, don't include extraData
Notice the then missing from the last sentence which starts with don't instead of then don't. 🤷
There was a problem hiding this comment.
Gotcha, so it was the name - updated 🙌
parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt
Outdated
Show resolved
Hide resolved
parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt
Outdated
Show resolved
Hide resolved
parsely/src/test/java/com/parsely/parselyandroid/EventsBuilderTest.kt
Outdated
Show resolved
Hide resolved
Description
This PR extracts logic of building events (pixels) and covers it with unit tests. It doesn't introduce any change in terms of how SDK works.
Tests
No need - review and CI check should be enough.