Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(827)

Issue 29347315: Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by anton
Modified:
1 month, 2 weeks ago
CC:
René Jeschke
Visibility:
Public.

Description

Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback A new version of changes is here: https://codereview.adblockplus.org/29424615/

Patch Set 1 #

Total comments: 2

Patch Set 2 : Introduced file system abstraction to java and using LazyFileSystem for filter tests #

Total comments: 3

Patch Set 3 : fix for firstRun test #

Total comments: 2

Patch Set 4 : patchset 2 + patchset 3 + applied Eyeo's codestyle #

Patch Set 5 : made helper methods static, fixed 'remove' for fs callback #

Total comments: 20
Unified diffs Side-by-side diffs Delta from patch set Stats (+1286 lines, -360 lines) Patch
A libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java View 1 2 3 1 chunk +295 lines, -0 lines 10 comments Download
M libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java View 1 2 3 1 chunk +10 lines, -9 lines 3 comments Download
M libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java View 1 2 3 2 chunks +357 lines, -345 lines 0 comments Download
A libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java View 1 2 3 1 chunk +84 lines, -0 lines 0 comments Download
M libadblockplus-android/jni/Android.mk View 1 3 1 chunk +1 line, -1 line 0 comments Download
M libadblockplus-android/jni/JniCallbacks.h View 1 3 1 chunk +15 lines, -0 lines 0 comments Download
A libadblockplus-android/jni/JniFileSystem.cpp View 1 2 3 4 1 chunk +183 lines, -0 lines 0 comments Download
M libadblockplus-android/jni/JniJsEngine.cpp View 1 3 2 chunks +14 lines, -0 lines 0 comments Download
M libadblockplus-android/jni/Utils.h View 1 3 1 chunk +2 lines, -0 lines 0 comments Download
M libadblockplus-android/jni/Utils.cpp View 1 3 2 chunks +12 lines, -0 lines 0 comments Download
A libadblockplus-android/src/org/adblockplus/android/AndroidFileSystem.java View 1 2 3 1 chunk +142 lines, -0 lines 2 comments Download
A + libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java View 1 2 3 2 chunks +49 lines, -5 lines 0 comments Download
A libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java View 1 2 3 4 1 chunk +115 lines, -0 lines 2 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java View 1 3 2 chunks +7 lines, -0 lines 3 comments Download

Messages

Total messages: 11
anton
Since JsEngine has built-in subscriptions list (easylist at least) we are having "downloading started" event ...
1 year, 1 month ago (2016-07-07 10:29:34 UTC) #1
sergei
https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java (right): https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java#newcode38 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:38: while (filterEngine.getListedSubscriptions().size() > 0) It seems there is a ...
1 year, 1 month ago (2016-07-07 12:48:42 UTC) #2
anton
https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java (right): https://codereview.adblockplus.org/29347315/diff/29347316/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java#newcode38 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: ...
1 year, 1 month ago (2016-07-07 13:33:54 UTC) #3
anton
switched to solution 3 - create file system abstraction and use it in tests (using ...
1 year, 1 month ago (2016-07-10 10:53:12 UTC) #4
anton
https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemHelper.java File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemHelper.java (right): https://codereview.adblockplus.org/29347315/diff/29347470/libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemHelper.java#newcode28 libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemHelper.java:28: public class FileSystemHelper could be called 'FileSystemUtils' in order ...
1 year, 1 month ago (2016-07-12 10:49:41 UTC) #5
René Jeschke
https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java (right): https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java#newcode24 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java:24: private boolean patternsIniExists = true; Is there a point ...
1 year, 1 month ago (2016-07-12 16:00:42 UTC) #6
anton
https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java (right): https://codereview.adblockplus.org/29347315/diff/29347530/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java#newcode24 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/LazyFileSystem.java:24: private boolean patternsIniExists = true; On 2016/07/12 16:00:42, René ...
1 year, 1 month ago (2016-07-13 06:41:14 UTC) #7
anton
https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java (right): https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java#newcode34 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:34: jsEngine.setFileSystem(new LazyFileSystem()); BTW i'm following "Lazy.." name rule practice ...
8 months, 1 week ago (2016-12-13 09:41:36 UTC) #8
anton
On 2016/12/13 09:41:36, anton wrote: > https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java > (right): > > ...
4 months, 1 week ago (2017-04-14 08:18:57 UTC) #9
diegocarloslima
https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java (right): https://codereview.adblockplus.org/29347315/diff/29367301/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java#newcode35 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:35: protected final static String DATA = "12345qwerty"; By our ...
3 months, 3 weeks ago (2017-04-27 12:22:40 UTC) #10
anton
3 months, 3 weeks ago (2017-04-28 08:25:46 UTC) #11
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5