Issue 5931 - Create tests for util package

I would prefer to adjust the project structure to what you get when you start a new project with android studio. Therefor I needed to move all java files so src/main/java.

At the moment I have no idea how we could run instrumentation test with espresso for ABP for Samsung. We cannot mock the static methods of the Engine class to emulate an environment with a Samsung Browser installed. It might make sense to refactor our code to have a better tests coverage, but I think we should start with unit tests first.

[anton, 2017-11-13 06:03:33]

On 2017/11/10 14:37:36, jens wrote:

First note - though you can use PowerMockito to mock static class members in
general it seems to be not testable design so i agree we should think about
making it more testable.
Let's say you can introduce Engine interface and factory to build it and real
and test factory impl. Just a thought.

Second - if you just moved the files, where are deleted files?

[diegocarloslima, 2017-11-13 12:35:30]

On 2017/11/13 06:03:33, anton wrote:
> On 2017/11/10 14:37:36, jens wrote:
> 
> First note - though you can use PowerMockito to mock static class members in
> general it seems to be not testable design so i agree we should think about
> making it more testable.
> Let's say you can introduce Engine interface and factory to build it and real
> and test factory impl. Just a thought.
> 
> Second - if you just moved the files, where are deleted files?

You need to use `hg mv` to move the files, otherwise the history of the file
will be lost

[diegocarloslima, 2017-11-13 21:10:13]

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
File
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
(right):

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:33:
import java.util.*
I don't really like using wildcards on imports

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:50:
.openRawResource(R.raw.subscriptions).use({ subscriptionsXml ->
mockedDefaultSubs =
Is this parenthesis in .use( really necessary?

[anton, 2017-11-14 06:04:10]

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
File
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
(right):

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:40:
private var context: Context? = null
it would be better to have it marked with `lateinit` and be not nullable IMHO:
`lateinit val context: Context`

[jens, 2017-11-14 10:50:24]

On 2017/11/13 06:03:33, anton wrote:
> On 2017/11/10 14:37:36, jens wrote:
> 
> First note - though you can use PowerMockito to mock static class members in
> general it seems to be not testable design so i agree we should think about
> making it more testable.
> Let's say you can introduce Engine interface and factory to build it and real
> and test factory impl. Just a thought.
> 
> Second - if you just moved the files, where are deleted files?

Anton: I looked into PowerMockito before and decided to not use it. Another
point is, that it uses byte code manipulation and there are no tools to make it
work with dalvik
So we cannot use it with Espresso for istrumentation tests. About the files: I
did not use hg mv to move them. I'll try to adjust it.

[jens, 2017-11-14 10:50:45]

On 2017/11/13 12:35:30, diegocarloslima wrote:
> On 2017/11/13 06:03:33, anton wrote:
> > On 2017/11/10 14:37:36, jens wrote:
> > 
> > First note - though you can use PowerMockito to mock static class members in
> > general it seems to be not testable design so i agree we should think about
> > making it more testable.
> > Let's say you can introduce Engine interface and factory to build it and
real
> > and test factory impl. Just a thought.
> > 
> > Second - if you just moved the files, where are deleted files?
> 
> You need to use `hg mv` to move the files, otherwise the history of the file
> will be lost

Thanks for pointing that out.

[jens, 2017-11-14 10:57:59]

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
File
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
(right):

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:33:
import java.util.*
On 2017/11/13 21:10:13, diegocarloslima wrote:
> I don't really like using wildcards on imports

Acknowledged.

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:40:
private var context: Context? = null
On 2017/11/14 06:04:09, anton wrote:
> it would be better to have it marked with `lateinit` and be not nullable IMHO:
> `lateinit val context: Context`

Acknowledged.

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:50:
.openRawResource(R.raw.subscriptions).use({ subscriptionsXml ->
mockedDefaultSubs =
On 2017/11/13 21:10:13, diegocarloslima wrote:
> Is this parenthesis in .use( really necessary?

No it's not. Good catch.

[jens, 2017-11-14 11:03:55]

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
File
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
(right):

https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:40:
private var context: Context? = null
On 2017/11/14 06:04:09, anton wrote:
> it would be better to have it marked with `lateinit` and be not nullable IMHO:
> `lateinit val context: Context`

But lateinit can only be used on var. So I will change it to private lateinit
var context: Context.

[jens, 2017-11-14 15:18:50]

On 2017/11/14 11:03:55, jens wrote:
>
https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
> File
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
> (right):
> 
>
https://codereview.adblockplus.org/29603697/diff/29603864/adblockplussbrowser...
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:40:
> private var context: Context? = null
> On 2017/11/14 06:04:09, anton wrote:
> > it would be better to have it marked with `lateinit` and be not nullable
IMHO:
> > `lateinit val context: Context`
> 
> But lateinit can only be used on var. So I will change it to private lateinit
> var context: Context.

I have uploaded a new patch set.
And thanks for your input to refactoring/extending Engine.java Anton. I'll keep
that in mind for extending the testing.

[anton, 2017-11-16 10:46:37]

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
File
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
(right):

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:38:

the line seems to be not required

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:39:
private var mockedDefaultSubs: DefaultSubscriptions? = null
shouldn't it be 'lateinit var .. ' too? it's init in `setup()`

[jens, 2017-11-17 09:16:26]

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
File
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
(right):

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:38:

On 2017/11/16 10:46:36, anton wrote:
> the line seems to be not required

Acknowledged.

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:39:
private var mockedDefaultSubs: DefaultSubscriptions? = null
On 2017/11/16 10:46:36, anton wrote:
> shouldn't it be 'lateinit var .. ' too? it's init in `setup()`

Yeah, you are right. That is the proper way to do in Kotlin. Still have to get
used to it.

[anton, 2017-11-17 11:59:34]

On 2017/11/17 09:16:26, jens wrote:
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> File
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
> (right):
> 
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:38:
> 
> On 2017/11/16 10:46:36, anton wrote:
> > the line seems to be not required
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:39:
> private var mockedDefaultSubs: DefaultSubscriptions? = null
> On 2017/11/16 10:46:36, anton wrote:
> > shouldn't it be 'lateinit var .. ' too? it's init in `setup()`
> 
> Yeah, you are right. That is the proper way to do in Kotlin. Still have to get
> used to it.

It's okay. i was a bit confused as you've refactored one variable to be
`lateinit` but did not do the same for the second variable.
So i just wondered if you did it intentionally or not.

[jens, 2017-11-21 09:16:24]

On 2017/11/17 11:59:34, anton wrote:
> On 2017/11/17 09:16:26, jens wrote:
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> > File
> >
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> >
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:38:
> > 
> > On 2017/11/16 10:46:36, anton wrote:
> > > the line seems to be not required
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> >
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:39:
> > private var mockedDefaultSubs: DefaultSubscriptions? = null
> > On 2017/11/16 10:46:36, anton wrote:
> > > shouldn't it be 'lateinit var .. ' too? it's init in `setup()`
> > 
> > Yeah, you are right. That is the proper way to do in Kotlin. Still have to
get
> > used to it.
> 
> It's okay. i was a bit confused as you've refactored one variable to be
> `lateinit` but did not do the same for the second variable.
> So i just wondered if you did it intentionally or not.

Yeah, last week I realized I should stay in bed when I feel sick and don't
work... It was not intentionally.
I uploaded another changeset where I deleted the empty line and changed
DefaultSubsciptions to lateinit.

[anton, 2017-11-21 09:37:04]

On 2017/11/21 09:16:24, jens wrote:
> On 2017/11/17 11:59:34, anton wrote:
> > On 2017/11/17 09:16:26, jens wrote:
> > >
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> > > File
> > >
> >
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt
> > > (right):
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> > >
> >
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:38:
> > > 
> > > On 2017/11/16 10:46:36, anton wrote:
> > > > the line seems to be not required
> > > 
> > > Acknowledged.
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> > >
> >
>
adblockplussbrowser/src/test/java/org/adblockplus/sbrowser/contentblocker/engine/UtilsTest.kt:39:
> > > private var mockedDefaultSubs: DefaultSubscriptions? = null
> > > On 2017/11/16 10:46:36, anton wrote:
> > > > shouldn't it be 'lateinit var .. ' too? it's init in `setup()`
> > > 
> > > Yeah, you are right. That is the proper way to do in Kotlin. Still have to
> get
> > > used to it.
> > 
> > It's okay. i was a bit confused as you've refactored one variable to be
> > `lateinit` but did not do the same for the second variable.
> > So i just wondered if you did it intentionally or not.
> 
> Yeah, last week I realized I should stay in bed when I feel sick and don't
> work... It was not intentionally.
> I uploaded another changeset where I deleted the empty line and changed
> DefaultSubsciptions to lateinit.

LGTM

[diegocarloslima, 2017-11-30 00:36:38]

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
File adblockplussbrowser/build.gradle (right):

https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
adblockplussbrowser/build.gradle:24: java.srcDirs = ['src/main/java/']
This can be omitted since it's now the default value. I was wondering if it's
worth to move the manifest and the resources to the default directories as well.
It was originally conceived this way, because we wanted to still keep the
project easily configurable in Eclipse, but I think we can move on, only
focusing on Android Studio. Maybe would be worth to split all this file moving
into another ticket and keep this review just for the changes directly related
to the util package testing

[jens, 2017-11-30 08:54:37]

On 2017/11/30 00:36:38, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> File adblockplussbrowser/build.gradle (right):
> 
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> adblockplussbrowser/build.gradle:24: java.srcDirs = ['src/main/java/']
> This can be omitted since it's now the default value. I was wondering if it's
> worth to move the manifest and the resources to the default directories as
well.
> It was originally conceived this way, because we wanted to still keep the
> project easily configurable in Eclipse, but I think we can move on, only
> focusing on Android Studio. Maybe would be worth to split all this file moving
> into another ticket and keep this review just for the changes directly related
> to the util package testing

Yeah, I already thought about it too. We should also remove sdkVersion,
versionName and versionNumber from the Manifest. I uploaded a new patch set
without srcDirs. Let's do the rest in a separate ticket.

[diegocarloslima, 2017-11-30 14:21:00]

On 2017/11/30 08:54:37, jens wrote:
> On 2017/11/30 00:36:38, diegocarloslima wrote:
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> > File adblockplussbrowser/build.gradle (right):
> > 
> >
>
https://codereview.adblockplus.org/29603697/diff/29607578/adblockplussbrowser...
> > adblockplussbrowser/build.gradle:24: java.srcDirs = ['src/main/java/']
> > This can be omitted since it's now the default value. I was wondering if
it's
> > worth to move the manifest and the resources to the default directories as
> well.
> > It was originally conceived this way, because we wanted to still keep the
> > project easily configurable in Eclipse, but I think we can move on, only
> > focusing on Android Studio. Maybe would be worth to split all this file
moving
> > into another ticket and keep this review just for the changes directly
related
> > to the util package testing
> 
> Yeah, I already thought about it too. We should also remove sdkVersion,
> versionName and versionNumber from the Manifest. I uploaded a new patch set
> without srcDirs. Let's do the rest in a separate ticket.

LGTM