Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback

Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback

A new version of changes is here: https://codereview.adblockplus.org/29424615/

[anton, 2016-07-07 10:29:34]

Since JsEngine has built-in subscriptions list (easylist at least) we are having
"downloading started" event callback which hurts testing - we expect to have
only 1 event callback, but in fact having 2.

We could solve it by:
1) filtering events in callback handler
or
2) preventing this events to happen at all (chosen now, clearing subscriptions
list in setUp)
or
3) override file system or copy default config file to return expected (empty)
subscriptions list.

Discussed with Sergey (suggested to override file system but it needs more deep
changes). Not sure worth for the testing only at the moment.

[sergei, 2016-07-07 12:48:42]

https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:38:
while (filterEngine.getListedSubscriptions().size() > 0)
It seems there is a race condition because the number of subscriptions can be
zero if they are not loaded yet. They can be loaded during the following
Thread.sleep for example.
So actually, firstly one needs to check the number of subscriptions, if it's
zero then wait for loading (using FilterNotifier.addListener, action == "load")
and only then one can remove them. If it's bigger than zero it means the loading
finished and they can be removed as it's done now.

Here is an example of waiting for the finishing of the loading of subscriptions
https://hg.adblockplus.org/abpcrawler/file/tip/lib/crawler.js#l214

[anton, 2016-07-07 13:33:54]

https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:38:
while (filterEngine.getListedSubscriptions().size() > 0)
On 2016/07/07 12:48:42, sergei wrote:
> It seems there is a race condition because the number of subscriptions can be
> zero if they are not loaded yet. They can be loaded during the following
> Thread.sleep for example.
> So actually, firstly one needs to check the number of subscriptions, if it's
> zero then wait for loading (using FilterNotifier.addListener, action ==
"load")
> and only then one can remove them. If it's bigger than zero it means the
loading
> finished and they can be removed as it's done now.
> 
> Here is an example of waiting for the finishing of the loading of
subscriptions
> https://hg.adblockplus.org/abpcrawler/file/tip/lib/crawler.js#l214

ok, since it's not good enough solution for the case #2 i'd prefer to prevent
default subscriptions to be loaded at all and switch to solution #3.

[anton, 2016-07-10 10:53:12]

switched to solution 3 - create file system abstraction and use it in tests
(using lazy file system to override subscriptions list and prevent loading them
in order not to hurt tests - otherwise it creates "subscription loading started"
events)

https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-andr...
File libadblockplus-android/jni/JniFileSystem.cpp (right):

https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-andr...
libadblockplus-android/jni/JniFileSystem.cpp:60: std::string cData =
JniJavaToStdString(*env, jData);
some concern here - since it's extremely difficult (is it possible at all?) to
transfer stream over jni (map std::istream to java streams) we're returning
string in java, create std stream for it and return it in c++ side

https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-andr...
libadblockplus-android/jni/JniFileSystem.cpp:77: std::string cData =
JniStdStreamToStdString(data.get());
some concern here too - since it's extremely difficult (is it possible at all?)
to transfer stream over jni (map std::istream to java streams) we're reading all
the buffer from c++ stream into std::string and pass java string to java.

[anton, 2016-07-12 10:49:41]

https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemHelper.java
(right):

https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemHelper.java:28:
public class FileSystemHelper
could be called 'FileSystemUtils' in order to be consistent with existing
'Utils' class

[René Jeschke, 2016-07-12 16:00:42]

https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java
(right):

https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java:24:
private boolean patternsIniExists = true;
Is there a point in making this flag modifiable _after_ creation? If not, then I
would prefer having this flag being set by a ctor, and let the default ctor set
it to 'false'.

[anton, 2016-07-13 06:41:14]

https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java
(right):

https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java:24:
private boolean patternsIniExists = true;
On 2016/07/12 16:00:42, René Jeschke wrote:
> Is there a point in making this flag modifiable _after_ creation? If not, then
I
> would prefer having this flag being set by a ctor, and let the default ctor
set
> it to 'false'.

It's unknown if we need to change it after creation during object lifetime. I've
been thinking about introducing it as ctor argument but i decided that later we
may need other variables to be set that's why i decided not to introduce this
limitation before we know we need it for sure. For current requirements we can
definitely do it if you insist. Do you?

[anton, 2016-12-13 09:41:36]

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:34:
jsEngine.setFileSystem(new LazyFileSystem());
BTW i'm following "Lazy.." name rule practice but actually it's not Lazy, it's
Stub or smth.. Should not we change it everywhere?

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:81:
public void setFileSystem(FileSystem fileSystem)
Just noting that it's extremely bad for performance to use this file system as
all read/write routines are inefficient because of stream to string copying to
pass JNI layer. Use 'setDefaultFile' for better performance. Setting test file
system if more efficient

[anton, 2017-04-14 08:18:57]

On 2016/12/13 09:41:36, anton wrote:
>
https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
> File
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:34:
> jsEngine.setFileSystem(new LazyFileSystem());
> BTW i'm following "Lazy.." name rule practice but actually it's not Lazy, it's
> Stub or smth.. Should not we change it everywhere?
> 
>
https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
> File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
> (right):
> 
>
https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
> libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:81:
> public void setFileSystem(FileSystem fileSystem)
> Just noting that it's extremely bad for performance to use this file system as
> all read/write routines are inefficient because of stream to string copying to
> pass JNI layer. Use 'setDefaultFile' for better performance. Setting test file
> system if more efficient

any chance to get it reviewed?

[diegocarloslima, 2017-04-27 12:22:40]

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:35:
protected final static String DATA = "12345qwerty";
By our code style, the modifier order should be 'static final'

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:107:
final String SINGLE_LINE_DATA = DATA.replace("\n", "");
by the convention, only static final variables (class constants) should have
this uppercase notation when final

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:121:
final String MULTI_LINE_DATA = DATA + "\n" + DATA;
by the convention, only static final variables (class constants) should have
this uppercase notation when final

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:148:
final String SINGLE_LINE_DATA = DATA.replace("\n", "");
by the convention, only static final variables (class constants) should have
this uppercase notation when final

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:162:
final String MULTI_LINE_DATA = DATA + "\n" + DATA;
by the convention, only static final variables (class constants) should have
this uppercase notation when final

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:34:
jsEngine.setFileSystem(new LazyFileSystem());
On 2016/12/13 09:41:36, anton wrote:
> BTW i'm following "Lazy.." name rule practice but actually it's not Lazy, it's
> Stub or smth.. Should not we change it everywhere?

Yeah, I do agree we should rename it, if it makes more sense. You can change the
'LazyFileSystem' in this issue and then create a follow up issue to change the
others.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/android/AndroidFileSystem.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/android/AndroidFileSystem.java:48:
this();
This call isn't required

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:81:
fos.close();
Are we using Java 7 already in libadblockplus-android? This could be done with a
try-with-resources statement

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:81:
public void setFileSystem(FileSystem fileSystem)
On 2016/12/13 09:41:36, anton wrote:
> Just noting that it's extremely bad for performance to use this file system as
> all read/write routines are inefficient because of stream to string copying to
> pass JNI layer. Use 'setDefaultFile' for better performance. Setting test file
> system if more efficient

I do agree that reading all the file and passing it as string is really
inefficient and can even cause OOM error. Couldn't we just pass the file path
(or even a path for a copy of the file itself) and have it read by the c++ side?

[anton, 2017-04-28 08:25:46]

Please switch to new review https://codereview.adblockplus.org/29424615/ instead
of this one

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:35:
protected final static String DATA = "12345qwerty";
On 2017/04/27 12:22:39, diegocarloslima wrote:
> By our code style, the modifier order should be 'static final'

Done.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:107:
final String SINGLE_LINE_DATA = DATA.replace("\n", "");
On 2017/04/27 12:22:39, diegocarloslima wrote:
> by the convention, only static final variables (class constants) should have
> this uppercase notation when final

Done.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:121:
final String MULTI_LINE_DATA = DATA + "\n" + DATA;
On 2017/04/27 12:22:39, diegocarloslima wrote:
> by the convention, only static final variables (class constants) should have
> this uppercase notation when final

Done.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:148:
final String SINGLE_LINE_DATA = DATA.replace("\n", "");
On 2017/04/27 12:22:39, diegocarloslima wrote:
> by the convention, only static final variables (class constants) should have
> this uppercase notation when final

Done.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:162:
final String MULTI_LINE_DATA = DATA + "\n" + DATA;
On 2017/04/27 12:22:40, diegocarloslima wrote:
> by the convention, only static final variables (class constants) should have
> this uppercase notation when final

Done.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:34:
jsEngine.setFileSystem(new LazyFileSystem());
On 2017/04/27 12:22:40, diegocarloslima wrote:
> On 2016/12/13 09:41:36, anton wrote:
> > BTW i'm following "Lazy.." name rule practice but actually it's not Lazy,
it's
> > Stub or smth.. Should not we change it everywhere?
> 
> Yeah, I do agree we should rename it, if it makes more sense. You can change
the
> 'LazyFileSystem' in this issue and then create a follow up issue to change the
> others.

Done.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/android/AndroidFileSystem.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/android/AndroidFileSystem.java:48:
this();
On 2017/04/27 12:22:40, diegocarloslima wrote:
> This call isn't required

I think it's required as if we have some init logics in default ctor it will not
be invoked without `this();`
Since we do have explicit default ctor we should invoke it explicitly.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:81:
fos.close();
On 2017/04/27 12:22:40, diegocarloslima wrote:
> Are we using Java 7 already in libadblockplus-android? This could be done with
a
> try-with-resources statement

it's not clear but seems that "not yet". For now i don't use Java 7 features
without special need for it.

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java
(right):

https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:81:
public void setFileSystem(FileSystem fileSystem)
On 2017/04/27 12:22:40, diegocarloslima wrote:
> On 2016/12/13 09:41:36, anton wrote:
> > Just noting that it's extremely bad for performance to use this file system
as
> > all read/write routines are inefficient because of stream to string copying
to
> > pass JNI layer. Use 'setDefaultFile' for better performance. Setting test
file
> > system if more efficient
> 
> I do agree that reading all the file and passing it as string is really
> inefficient and can even cause OOM error. Couldn't we just pass the file path
> (or even a path for a copy of the file itself) and have it read by the c++
side?

Then it removes the sense of providing the impl at all.
eg. how will override returning file content in Java code?
I believe the hierarchy and android impl is okay but i've added a comment to
AndroidFileSystem class about it