Fix libadblockplus-android tests

Fix libadblockplus-android tests

[anton, 2018-07-11 13:57:31]

On 2018/07/11 13:37:04, René Jeschke wrote:

What do you think about using mockito or powermock?

[anton, 2018-07-11 13:58:31]

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
int prev =  filterEngine.getListedFilters().size();
extra not required space

[anton, 2018-07-11 14:02:44]

On 2018/07/11 13:58:31, anton wrote:
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
> int prev =  filterEngine.getListedFilters().size();
> extra not required space

It seems some changes are required after
https://github.com/adblockplus/libadblockplus-android/commit/af2df3173961798e...
landed.

[anton, 2018-07-16 06:58:53]

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:32:
public class FilterEngineTest extends BaseFilterEngineTest
i was unable to apply diff patch so just looking the sources.
Since `BaseFilterEngineTest` is deleted
(https://codereview.adblockplus.org/29819555/patch/29819556/29819558) how can
this file be compiled?

What about deleted `BasePlatformTest`
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
?

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:377:

i still can see first run flag test in c++ source:
https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c...

but here it's just deleted.

[anton, 2018-07-23 07:13:46]

On 2018/07/16 06:58:53, anton wrote:
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:32:
> public class FilterEngineTest extends BaseFilterEngineTest
> i was unable to apply diff patch so just looking the sources.
> Since `BaseFilterEngineTest` is deleted
> (https://codereview.adblockplus.org/29819555/patch/29819556/29819558) how can
> this file be compiled?
> 
> What about deleted `BasePlatformTest`
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> ?
> 
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:377:
> 
> i still can see first run flag test in c++ source:
>
https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c...
> 
> but here it's just deleted.

any update on this?

[René Jeschke, 2018-08-16 11:47:06]

On 2018/07/23 07:13:46, anton wrote:
> On 2018/07/16 06:58:53, anton wrote:
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> > File
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:32:
> > public class FilterEngineTest extends BaseFilterEngineTest
> > i was unable to apply diff patch so just looking the sources.
> > Since `BaseFilterEngineTest` is deleted
> > (https://codereview.adblockplus.org/29819555/patch/29819556/29819558) how
can
> > this file be compiled?
> > 
> > What about deleted `BasePlatformTest`
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> > ?
> > 
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:377:
> > 
> > i still can see first run flag test in c++ source:
> >
>
https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c...
> > 
> > but here it's just deleted.
> 
> any update on this?

'upload.py' seems to have missed a few files, will try to re-upload.

[René Jeschke, 2018-08-16 11:47:15]

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:32:
public class FilterEngineTest extends BaseFilterEngineTest
On 2018/07/16 06:58:52, anton wrote:
> i was unable to apply diff patch so just looking the sources.
> Since `BaseFilterEngineTest` is deleted
> (https://codereview.adblockplus.org/29819555/patch/29819556/29819558) how can
> this file be compiled?
> 
> What about deleted `BasePlatformTest`
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> ?

Yeah, somehow 'upload.py' missed that I moved the files ... will see if a
re-upload will help.

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
int prev =  filterEngine.getListedFilters().size();
On 2018/07/11 13:58:30, anton wrote:
> extra not required space

Done.

https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:377:

On 2018/07/16 06:58:52, anton wrote:
> i still can see first run flag test in c++ source:
>
https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c...
> 
> but here it's just deleted.

There's an extra test for the first run flag:
org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest

[René Jeschke, 2018-08-16 12:03:06]

On 2018/08/16 11:47:15, René Jeschke wrote:
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:32:
> public class FilterEngineTest extends BaseFilterEngineTest
> On 2018/07/16 06:58:52, anton wrote:
> > i was unable to apply diff patch so just looking the sources.
> > Since `BaseFilterEngineTest` is deleted
> > (https://codereview.adblockplus.org/29819555/patch/29819556/29819558) how
can
> > this file be compiled?
> > 
> > What about deleted `BasePlatformTest`
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> > ?
> 
> Yeah, somehow 'upload.py' missed that I moved the files ... will see if a
> re-upload will help.
> 
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
> int prev =  filterEngine.getListedFilters().size();
> On 2018/07/11 13:58:30, anton wrote:
> > extra not required space
> 
> Done.
> 
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:377:
> 
> On 2018/07/16 06:58:52, anton wrote:
> > i still can see first run flag test in c++ source:
> >
>
https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c...
> > 
> > but here it's just deleted.
> 
> There's an extra test for the first run flag:
> org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest

So, re-uploading doesn't fix anything, here's the branch at Github:
https://github.com/adblockplus/libadblockplus-android/compare/master...rjesch...

[René Jeschke, 2018-08-16 12:13:56]

On 2018/08/16 12:03:06, René Jeschke wrote:
> On 2018/08/16 11:47:15, René Jeschke wrote:
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> > File
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:32:
> > public class FilterEngineTest extends BaseFilterEngineTest
> > On 2018/07/16 06:58:52, anton wrote:
> > > i was unable to apply diff patch so just looking the sources.
> > > Since `BaseFilterEngineTest` is deleted
> > > (https://codereview.adblockplus.org/29819555/patch/29819556/29819558) how
> can
> > > this file be compiled?
> > > 
> > > What about deleted `BasePlatformTest`
> > >
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> > > ?
> > 
> > Yeah, somehow 'upload.py' missed that I moved the files ... will see if a
> > re-upload will help.
> > 
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
> > int prev =  filterEngine.getListedFilters().size();
> > On 2018/07/11 13:58:30, anton wrote:
> > > extra not required space
> > 
> > Done.
> > 
> >
>
https://codereview.adblockplus.org/29819555/diff/29819556/adblock-android-tes...
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:377:
> > 
> > On 2018/07/16 06:58:52, anton wrote:
> > > i still can see first run flag test in c++ source:
> > >
> >
>
https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c...
> > > 
> > > but here it's just deleted.
> > 
> > There's an extra test for the first run flag:
> > org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest
> 
> So, re-uploading doesn't fix anything, here's the branch at Github:
>
https://github.com/adblockplus/libadblockplus-android/compare/master...rjesch...

Ahhh ... "libadblockplus" is in the .gitignore ... need to rename the packages,
then I'll re-upload the diff. :D

[René Jeschke, 2018-08-16 12:28:24]

Complete patch set is up.

[anton, 2018-08-24 12:25:04]

rene, could you please add to "CC" diego and jens too (i have no edit rights)?

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:18:
package org.adblockplus.libadblockplus;
Why is it moved from `tests` package?
Same about all other moved `Base..` tests: `BaseFilterEngineTest.java`,
`BaseJsEngineTest.java`, `BasePlatformTest.java`, `BaseTest.java`,
`UpdaterTest.java`.
Was it intentionally?

The original idea was to have test stub/impls in regular package and tests
classes in `test` package.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:81: {
wrong indentation

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:86: {
wrong indentation

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:91: {
wrong indentation

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:96: {
wrong indentation

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:105:
protected void deleteFiles()
let's move it to `Util` or `Helper` class

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:147: File
patterns = new File(getContext().getFilesDir(), "patterns.ini");
It sounds like we know too much of `libadblockplus` implementation details.
Let's say filename will be changed one day and the test fails, though it should
not as everything still works.

How do you think?

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
int prev = filterEngine.getListedFilters().size();
Can you please explain what's happening here?
Is it to make sure `removeFromList()` really deletes a filter?

Originally it was just removing all the filters until we have it.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:127:
while (filterEngine.getListedSubscriptions().size() > 0)
copy-paste from above. Can you create separate function like `clearFilters()`
and call it from there and here?
I agree it was also copy-paster before but it was 2 lines and now it's much
more.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/MockWebRequestTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/MockWebRequestTest.java:80:
} catch (AdblockPlusException e)
new line required here

[René Jeschke, 2018-08-27 20:02:16]

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:18:
package org.adblockplus.libadblockplus;
On 2018/08/24 12:25:03, anton wrote:
> Why is it moved from `tests` package?
> Same about all other moved `Base..` tests: `BaseFilterEngineTest.java`,
> `BaseJsEngineTest.java`, `BasePlatformTest.java`, `BaseTest.java`,
> `UpdaterTest.java`.
> Was it intentionally?
> 
> The original idea was to have test stub/impls in regular package and tests
> classes in `test` package.

Yes, it was intentionally. Those four classes don't test anything as they are
base classes which only make sense when subclassed. This was the reason why I
moved them one up. As the package is named 'tests' I only wanted tests to be in
there. I can, of course, move them back if you insist.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:81: {
On 2018/08/24 12:25:03, anton wrote:
> wrong indentation

Done.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:86: {
On 2018/08/24 12:25:03, anton wrote:
> wrong indentation

Done.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:91: {
On 2018/08/24 12:25:04, anton wrote:
> wrong indentation

Done.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:96: {
On 2018/08/24 12:25:03, anton wrote:
> wrong indentation

Done.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:105:
protected void deleteFiles()
On 2018/08/24 12:25:03, anton wrote:
> let's move it to `Util` or `Helper` class

It is only used here and in the context of tests. Do we gain anything if we move
this method to a helper class?

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:147: File
patterns = new File(getContext().getFilesDir(), "patterns.ini");
On 2018/08/24 12:25:04, anton wrote:
> It sounds like we know too much of `libadblockplus` implementation details.
> Let's say filename will be changed one day and the test fails, though it
should
> not as everything still works.
> 
> How do you think?

You got a point, but libadblockplus does not implement any kind of event
callback system for getting any information about it's state. There are tests
that depend on a clean state. The solution you used before was using 'sleep(n)',
but this had two issues: any timeout you specify can be too short, it makes the
test use an unbearable amount of time to complete. Going back to the previous
state would mean getting back to unreliable tests which may fail without any
(real) reason.

If you know any other reliable way to query libadblockplus' persistence state,
then I will gladly remove this. Until then, this method provides a reliable way
to make the tests deterministic. Also, a file name change is more than unlikely,
as the file name is in valid 8.3 format and so supported by any file system that
I am aware of.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
int prev = filterEngine.getListedFilters().size();
On 2018/08/24 12:25:04, anton wrote:
> Can you please explain what's happening here?
> Is it to make sure `removeFromList()` really deletes a filter?
> 
> Originally it was just removing all the filters until we have it.

There was a change in the way subscriptions work in libapb. The AA subscription
can not be removed from listed subscriptions as soon as it has been enabled.
Disabling is possible, but it will stay listed. This caused many tests to fail
or run in endless loops. The 'prev' thing now checks if we really removed a
subscription and breaks the loop otherwise. So we only end up with failing test
in the worst case and not endless loops.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:127:
while (filterEngine.getListedSubscriptions().size() > 0)
On 2018/08/24 12:25:04, anton wrote:
> copy-paste from above. Can you create separate function like `clearFilters()`
> and call it from there and here?
> I agree it was also copy-paster before but it was 2 lines and now it's much
> more.

Done.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/MockWebRequestTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/MockWebRequestTest.java:80:
} catch (AdblockPlusException e)
On 2018/08/24 12:25:04, anton wrote:
> new line required here

Done.

[René Jeschke, 2018-08-27 20:16:37]

PS to the 'patterns.ini' issue.

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
File adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:147: File
patterns = new File(getContext().getFilesDir(), "patterns.ini");
On 2018/08/27 20:02:15, René Jeschke wrote:
> On 2018/08/24 12:25:04, anton wrote:
> > It sounds like we know too much of `libadblockplus` implementation details.
> > Let's say filename will be changed one day and the test fails, though it
> should
> > not as everything still works.
> > 
> > How do you think?
> 
> You got a point, but libadblockplus does not implement any kind of event
> callback system for getting any information about it's state. There are tests
> that depend on a clean state. The solution you used before was using
'sleep(n)',
> but this had two issues: any timeout you specify can be too short, it makes
the
> test use an unbearable amount of time to complete. Going back to the previous
> state would mean getting back to unreliable tests which may fail without any
> (real) reason.
> 
> If you know any other reliable way to query libadblockplus' persistence state,
> then I will gladly remove this. Until then, this method provides a reliable
way
> to make the tests deterministic. Also, a file name change is more than
unlikely,
> as the file name is in valid 8.3 format and so supported by any file system
that
> I am aware of.

As you said that everything 'still works', did you test it? On an emulator or a
real device? Here's what I get when I remove this check and run the tests on an
emulator:

org.adblockplus.libadblockplus.tests.AndroidWebRequestResourceWrapperTest >
testNotIntercepted_Easylist[Nexus_5X_API_27_x86(AVD) - 8.1.0] FAILED 
        junit.framework.AssertionFailedError: expected:<1> but was:<2>
        at
org.adblockplus.libadblockplus.tests.AndroidWebRequestResourceWrapperTest.testNotIntercepted(AndroidWebRequestResourceWrapperTest.java:241)

org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest >
testFirstRunClearWithData[Nexus_5X_API_27_x86(AVD) - 8.1.0] FAILED 
        junit.framework.AssertionFailedError
        at
org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest.testFirstRunClearWithData(FilterEngineFirstRunTest.java:48)

FAILURE: Build failed with an exception.

[anton, 2018-08-28 07:03:55]

On 2018/08/27 20:02:16, René Jeschke wrote:
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:18:
> package org.adblockplus.libadblockplus;
> On 2018/08/24 12:25:03, anton wrote:
> > Why is it moved from `tests` package?
> > Same about all other moved `Base..` tests: `BaseFilterEngineTest.java`,
> > `BaseJsEngineTest.java`, `BasePlatformTest.java`, `BaseTest.java`,
> > `UpdaterTest.java`.
> > Was it intentionally?
> > 
> > The original idea was to have test stub/impls in regular package and tests
> > classes in `test` package.
> 
> Yes, it was intentionally. Those four classes don't test anything as they are
> base classes which only make sense when subclassed. This was the reason why I
> moved them one up. As the package is named 'tests' I only wanted tests to be
in
> there. I can, of course, move them back if you insist.
> 

It's still test classes - not test mocks/stubs of production classes, though
they are base classes.
Let's move them to `tests` package.

>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> File adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:81: {
> On 2018/08/24 12:25:03, anton wrote:
> > wrong indentation
> 
> Done.
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:86: {
> On 2018/08/24 12:25:03, anton wrote:
> > wrong indentation
> 
> Done.
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:91: {
> On 2018/08/24 12:25:04, anton wrote:
> > wrong indentation
> 
> Done.
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:96: {
> On 2018/08/24 12:25:03, anton wrote:
> > wrong indentation
> 
> Done.
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:105:
> protected void deleteFiles()
> On 2018/08/24 12:25:03, anton wrote:
> > let's move it to `Util` or `Helper` class
> 
> It is only used here and in the context of tests. Do we gain anything if we
move
> this method to a helper class?

In current context we will not gain anything, but in general i'd prefer to move
it.
The reason is that when it's needed by somebody else one could just search for
some `Helper` classes and reuse it.
If nobody knows this functionality is already implemented here it will be just
double-work to write it.
I'm not strong about it but for me personally i'd prefer to extract a class.

> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:147:
File
> patterns = new File(getContext().getFilesDir(), "patterns.ini");
> On 2018/08/24 12:25:04, anton wrote:
> > It sounds like we know too much of `libadblockplus` implementation details.
> > Let's say filename will be changed one day and the test fails, though it
> should
> > not as everything still works.
> > 
> > How do you think?
> 
> You got a point, but libadblockplus does not implement any kind of event
> callback system for getting any information about it's state.

That's true.

> There are tests
> that depend on a clean state. The solution you used before was using
'sleep(n)',
> but this had two issues: any timeout you specify can be too short, it makes
the
> test use an unbearable amount of time to complete. Going back to the previous
> state would mean getting back to unreliable tests which may fail without any
> (real) reason.
> 
> If you know any other reliable way to query libadblockplus' persistence state,
> then I will gladly remove this. Until then, this method provides a reliable
way
> to make the tests deterministic. Also, a file name change is more than
unlikely,
> as the file name is in valid 8.3 format and so supported by any file system
that
> I am aware of.

I would not say it can be changed because of file system limitation, but just
because `.ini` sounds natural for
windows but strange for android. I tend to agree with you in general, so let's
leave it as you did it.

> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:56:
> int prev = filterEngine.getListedFilters().size();
> On 2018/08/24 12:25:04, anton wrote:
> > Can you please explain what's happening here?
> > Is it to make sure `removeFromList()` really deletes a filter?
> > 
> > Originally it was just removing all the filters until we have it.
> 
> There was a change in the way subscriptions work in libapb. The AA
subscription
> can not be removed from listed subscriptions as soon as it has been enabled.
> Disabling is possible, but it will stay listed. This caused many tests to fail
> or run in endless loops. The 'prev' thing now checks if we really removed a
> subscription and breaks the loop otherwise. So we only end up with failing
test
> in the worst case and not endless loops.

That's clear, thanks for the explanation.

> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:127:
> while (filterEngine.getListedSubscriptions().size() > 0)
> On 2018/08/24 12:25:04, anton wrote:
> > copy-paste from above. Can you create separate function like
`clearFilters()`
> > and call it from there and here?
> > I agree it was also copy-paster before but it was 2 lines and now it's much
> > more.
> 
> Done.
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/MockWebRequestTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/MockWebRequestTest.java:80:
> } catch (AdblockPlusException e)
> On 2018/08/24 12:25:04, anton wrote:
> > new line required here
> 
> Done.

[anton, 2018-08-28 07:08:19]

On 2018/08/27 20:16:37, René Jeschke wrote:
> PS to the 'patterns.ini' issue.
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> File adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29857583/adblock-android-tes...
> adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java:147:
File
> patterns = new File(getContext().getFilesDir(), "patterns.ini");
> On 2018/08/27 20:02:15, René Jeschke wrote:
> > On 2018/08/24 12:25:04, anton wrote:
> > > It sounds like we know too much of `libadblockplus` implementation
details.
> > > Let's say filename will be changed one day and the test fails, though it
> > should
> > > not as everything still works.
> > > 
> > > How do you think?
> > 
> > You got a point, but libadblockplus does not implement any kind of event
> > callback system for getting any information about it's state. There are
tests
> > that depend on a clean state. The solution you used before was using
> 'sleep(n)',
> > but this had two issues: any timeout you specify can be too short, it makes
> the
> > test use an unbearable amount of time to complete. Going back to the
previous
> > state would mean getting back to unreliable tests which may fail without any
> > (real) reason.
> > 
> > If you know any other reliable way to query libadblockplus' persistence
state,
> > then I will gladly remove this. Until then, this method provides a reliable
> way
> > to make the tests deterministic. Also, a file name change is more than
> unlikely,
> > as the file name is in valid 8.3 format and so supported by any file system
> that
> > I am aware of.
> 
> As you said that everything 'still works', did you test it? On an emulator or
a
> real device? Here's what I get when I remove this check and run the tests on
an
> emulator:
> 
> org.adblockplus.libadblockplus.tests.AndroidWebRequestResourceWrapperTest >
> testNotIntercepted_Easylist[Nexus_5X_API_27_x86(AVD) - 8.1.0] FAILED 
>         junit.framework.AssertionFailedError: expected:<1> but was:<2>
>         at
>
org.adblockplus.libadblockplus.tests.AndroidWebRequestResourceWrapperTest.testNotIntercepted(AndroidWebRequestResourceWrapperTest.java:241)
> 
> org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest >
> testFirstRunClearWithData[Nexus_5X_API_27_x86(AVD) - 8.1.0] FAILED 
>         junit.framework.AssertionFailedError
>         at
>
org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest.testFirstRunClearWithData(FilterEngineFirstRunTest.java:48)
> 
> FAILURE: Build failed with an exception.

"everything still works" - i've meant "everything still works in libadblockplus
after renaming the file".
If filename is changed in libadblockplus all related code still continues to
work in libadblockplus
and our test will fail just because it takes into account some internal
implementation details that should be hidden form any consumers.
I did not run the tests and did not check they still run and succeed.

[anton, 2018-09-17 06:55:14]

https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:86:
filterEngine.getListedSubscriptions().get(0).removeFromList();
isn't it better to return `boolean` for `removeFromList` call (actually removed
or nor)? Probably it requires changes in `libadblockplus` module  though and can
be done in separate task..

[anton, 2018-09-17 06:56:10]

On 2018/09/17 06:55:14, anton wrote:
>
https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:86:
> filterEngine.getListedSubscriptions().get(0).removeFromList();
> isn't it better to return `boolean` for `removeFromList` call (actually
removed
> or nor)? Probably it requires changes in `libadblockplus` module  though and
can
> be done in separate task..

in general LGTM now (minor note about checking if subscription was actually
removed).

[anton, 2018-10-16 13:24:56]

https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:87:
if (prev == filterEngine.getListedSubscriptions().size())
this still looks suspicious.
sergey: "removing should always work".
NOT LGTM - we need to double check it.

[anton, 2018-10-18 11:40:14]

https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
File
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
(right):

https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:95:
}
the file does not correspond to last revision of file in github:
https://github.com/rjeschke/libadblockplus-android/blob/f46de56d2adab2c361916...

[anton, 2018-10-23 11:35:43]

On 2018/10/18 11:40:14, anton wrote:
>
https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
> File
>
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29819555/diff/29866603/adblock-android-tes...
>
adblock-android-tests/src/org/adblockplus/libadblockplus/BaseFilterEngineTest.java:95:
> }
> the file does not correspond to last revision of file in github:
>
https://github.com/rjeschke/libadblockplus-android/blob/f46de56d2adab2c361916...

rene, can you please:
1. rebase to
https://github.com/adblockplus/libadblockplus-android/commit/4527fc59760f47cb...
2. explain why subscirptions removal is done that way or change it