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

Issue 29347315: Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 3 weeks ago by anton
Modified:
12 hours, 37 minutes ago
CC:
René Jeschke
Visibility:
Public.

Description

Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback

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: 11
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 5 comments Download
M libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java View 1 2 3 1 chunk +10 lines, -9 lines 2 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 1 comment 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 1 comment Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java View 1 3 2 chunks +7 lines, -0 lines 2 comments Download

Messages

Total messages: 10
anton
Since JsEngine has built-in subscriptions list (easylist at least) we are having "downloading started" event ...
9 months, 3 weeks 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 ...
9 months, 3 weeks 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: ...
9 months, 3 weeks ago (2016-07-07 13:33:54 UTC) #3
anton
switched to solution 3 - create file system abstraction and use it in tests (using ...
9 months, 3 weeks 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 ...
9 months, 2 weeks 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 ...
9 months, 2 weeks 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é ...
9 months, 2 weeks 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 ...
4 months, 2 weeks 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): > > ...
1 week, 6 days ago (2017-04-14 08:18:57 UTC) #9
diegocarloslima
14 hours, 14 minutes ago (2017-04-27 12:22:40 UTC) #10
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?
Sign in to reply to this message.

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