Issue 5556 - Update to use libadblockplus revision hg:566f64c8a2a8

This change is mainly to simplify further reviews concerning #5556.

- rename BaseJsTest to BaseJsEngineTest and FilterEngineGenericTest to BaseFilterEngineTest for consistency among names of base test fixture classes. In the upcoming review there is going to appear yet one base test class.
- add BaseJsEngine.tearDown disposing JsEngine if it's instantiated.
- add BaseFilterEngine.tearDown disposing FilterEngine if it's instantiated. In addition it waits for some time in order to reduce the probability of crashing. The crash occurs because asynchronous operations of usually file system accomplish when FilterEngine and JsEngine are already destroyed. It won't be needed after finishing #5179 in libadblockplus and following updating of libadblockplus in libadblockplus-android. However, if one gets rid of race conditions in tests (see a lot of sleeps) by implementing asynchronous interfaces for tests with managed execution of operations, as it's currently done in libadblockplus, then that sleep will may go away as well.
- inherit from BaseFilterEngineTest if a test needs FilterEngine. It's not only to don't forget about disposing of corresponding classes and to describe that a test uses FilterEngine but it's also a step towards future changes (concerning above described race conditions) because fixture BaseFilterEngine will contain helpers and provide with additional structures.
- don't inherit from BaseJsEngineTest nor BaseFilterEngineTest when there is no necessity in it.

# depends on https://codereview.adblockplus.org/29536601/
COLLABORATOR=anton@adblockplus.org

[anton, 2017-09-06 06:33:48]

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:84:
jsEngine.evaluate(
is creating FilterEngine instance not required anymore? if yes, then comment
before class declaration is not required too..

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:122:
Thread.sleep(20 * 1000); // wait for the subscription to be downloaded
is creating FilterEngine instance not required anymore?

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java:46:
if (jsEngine != null)
original GC approach assumes that when Java instance if being GCed dispose
wrapper disposes related C++ instance automatically. So C++ instance is disposed
after Java instance is GCed.

Why is explicit GCing introduced here?

[sergei, 2017-09-07 10:33:31]

JIC, I have extracted these change from the final change in order to simplify
the review process and because smaller commits are always better. If one thinks
that it is really worth further splitting of this change, please let me know.

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:84:
jsEngine.evaluate(
On 2017/09/06 06:33:47, anton wrote:
> is creating FilterEngine instance not required anymore? if yes, then comment
> before class declaration is not required too..

The creation of FilterEngine is still required and it's done in `setUp`, that's
because the class fixture inherits BaseFilterEngineTest. In addition please
check the part of the commit message concerning inheritance from
BaseFilterEngineTest.

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java:46:
if (jsEngine != null)
On 2017/09/06 06:33:47, anton wrote:
> original GC approach assumes that when Java instance if being GCed dispose
> wrapper disposes related C++ instance automatically. So C++ instance is
disposed
> after Java instance is GCed.
> 
> Why is explicit GCing introduced here?

I think that manual disposing of JsEngine and FilterEngine in tearDown is a good
thing because of several reasons:
- we know the exact time when they are disposed, it means that if it crashes
during disposing or a bit later after the test, because some async operation has
just finished, we will know the exact test which led to the crash, not later
when it's already another test. In addition it can be also helpful for all sorts
of extra profiling, e.g. to see the memory consumption by a particular test (one
can easily detect a memory leakage) or to see the time needed by a test.
- although each test has its own instance of v8::Isolate V8 still shares some
internals between them, so for better tests isolation it would be better to free
all resources when a test finishes.
- ABP consumes some reasonable amount of memory, so theoretically if GC is
slightly slow then we can accumulate too much used memory and run out of memory.
It's actually not so theoretical, we have reached that point with ~100 tests in
libadblockplus on 32 bit paltform when we had infinite async operations (e.g. in
WebRequest) preventing JsEngine from being destroyed.

[anton, 2017-09-07 11:51:13]

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:84:
jsEngine.evaluate(
On 2017/09/07 10:33:31, sergei wrote:
> On 2017/09/06 06:33:47, anton wrote:
> > is creating FilterEngine instance not required anymore? if yes, then comment
> > before class declaration is not required too..
> 
> The creation of FilterEngine is still required and it's done in `setUp`,
that's
> because the class fixture inherits BaseFilterEngineTest. In addition please
> check the part of the commit message concerning inheritance from
> BaseFilterEngineTest.

Got it. Then we just don't need the comment before the class at all

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java:46:
if (jsEngine != null)
On 2017/09/07 10:33:31, sergei wrote:
> On 2017/09/06 06:33:47, anton wrote:
> > original GC approach assumes that when Java instance if being GCed dispose
> > wrapper disposes related C++ instance automatically. So C++ instance is
> disposed
> > after Java instance is GCed.
> > 
> > Why is explicit GCing introduced here?
> 
> I think that manual disposing of JsEngine and FilterEngine in tearDown is a
good
> thing because of several reasons:
> - we know the exact time when they are disposed, it means that if it crashes
> during disposing or a bit later after the test, because some async operation
has
> just finished, we will know the exact test which led to the crash, not later
> when it's already another test. In addition it can be also helpful for all
sorts
> of extra profiling, e.g. to see the memory consumption by a particular test
(one
> can easily detect a memory leakage) or to see the time needed by a test.
> - although each test has its own instance of v8::Isolate V8 still shares some
> internals between them, so for better tests isolation it would be better to
free
> all resources when a test finishes.
> - ABP consumes some reasonable amount of memory, so theoretically if GC is
> slightly slow then we can accumulate too much used memory and run out of
memory.
> It's actually not so theoretical, we have reached that point with ~100 tests
in
> libadblockplus on 32 bit paltform when we had infinite async operations (e.g.
in
> WebRequest) preventing JsEngine from being destroyed.

Well, it seems to be more C++ than Java approach it can bring benefits you've
mentioned.

[anton, 2017-09-07 11:51:53]

On 2017/09/07 11:51:13, anton wrote:
>
https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
> File
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:84:
> jsEngine.evaluate(
> On 2017/09/07 10:33:31, sergei wrote:
> > On 2017/09/06 06:33:47, anton wrote:
> > > is creating FilterEngine instance not required anymore? if yes, then
comment
> > > before class declaration is not required too..
> > 
> > The creation of FilterEngine is still required and it's done in `setUp`,
> that's
> > because the class fixture inherits BaseFilterEngineTest. In addition please
> > check the part of the commit message concerning inheritance from
> > BaseFilterEngineTest.
> 
> Got it. Then we just don't need the comment before the class at all
> 
>
https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
> File
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29536604/diff/29536605/libadblockplus-andr...
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsEngineTest.java:46:
> if (jsEngine != null)
> On 2017/09/07 10:33:31, sergei wrote:
> > On 2017/09/06 06:33:47, anton wrote:
> > > original GC approach assumes that when Java instance if being GCed dispose
> > > wrapper disposes related C++ instance automatically. So C++ instance is
> > disposed
> > > after Java instance is GCed.
> > > 
> > > Why is explicit GCing introduced here?
> > 
> > I think that manual disposing of JsEngine and FilterEngine in tearDown is a
> good
> > thing because of several reasons:
> > - we know the exact time when they are disposed, it means that if it crashes
> > during disposing or a bit later after the test, because some async operation
> has
> > just finished, we will know the exact test which led to the crash, not later
> > when it's already another test. In addition it can be also helpful for all
> sorts
> > of extra profiling, e.g. to see the memory consumption by a particular test
> (one
> > can easily detect a memory leakage) or to see the time needed by a test.
> > - although each test has its own instance of v8::Isolate V8 still shares
some
> > internals between them, so for better tests isolation it would be better to
> free
> > all resources when a test finishes.
> > - ABP consumes some reasonable amount of memory, so theoretically if GC is
> > slightly slow then we can accumulate too much used memory and run out of
> memory.
> > It's actually not so theoretical, we have reached that point with ~100 tests
> in
> > libadblockplus on 32 bit paltform when we had infinite async operations
(e.g.
> in
> > WebRequest) preventing JsEngine from being destroyed.
> 
> Well, it seems to be more C++ than Java approach it can bring benefits you've
> mentioned.

LGTM in general if comment is removed in test

[sergei, 2017-09-07 12:55:54]

https://codereview.adblockplus.org/29536604/diff/29538616/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29538616/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:34:
// FilterEngine.
As discussed in IRC, it's not removed, it's changed.

[anton, 2017-09-07 12:57:48]

https://codereview.adblockplus.org/29536604/diff/29538616/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
(right):

https://codereview.adblockplus.org/29536604/diff/29538616/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:34:
// FilterEngine.
On 2017/09/07 12:55:54, sergei wrote:
> As discussed in IRC, it's not removed, it's changed.

LGTM

[diegocarloslima, 2017-09-08 12:04:14]

On 2017/09/07 12:57:48, anton wrote:
>
https://codereview.adblockplus.org/29536604/diff/29538616/libadblockplus-andr...
> File
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29536604/diff/29538616/libadblockplus-andr...
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:34:
> // FilterEngine.
> On 2017/09/07 12:55:54, sergei wrote:
> > As discussed in IRC, it's not removed, it's changed.
> 
> LGTM

LGTM