Issue 4181 - Fix FilterEngineTest tests

Issue 4181 - Fix FilterEngineTest tests

#depends on https://codereview.adblockplus.org/29344967/

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

After discussion of the issues with sergey i've found that JsEngine stores it's
state persistently in file system and if the file system path is the same a
newly created instance loads the saved state from the previous instance.
Eventually it hurts tests as actual js state is different from expected one.

Fixed by generating new file system path for each test.

PS. Also fixes some other tests (i will add this in commit text).

[anton, 2016-08-08 07:52:52]

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:25:
public class FileSystemUtils
also renamed FileSystemHelper to FileSystemUtils to be consistent with other
..Utils classes already used

[diegocarloslima, 2016-09-09 01:34:15]

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:27:
private static final String TAG = FileSystemUtils.class.getSimpleName();
You might want to use Utils.getTag() to be consistent with other TAG variables
that uses the simple name of the class

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:49:
}
I think that it would be more consistent if both of the methods
'generateUniqueFileName' returns a File instead of one returning a String and
other a File. Also, since both would be always returning a File, it would make
more sense to be renamed to 'generateUniqueFile'

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:88:
}
Since none of these methods retains any state for the class instance, I think
that it would make more sense to all be static methods

[anton, 2016-09-09 06:50:53]

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:27:
private static final String TAG = FileSystemUtils.class.getSimpleName();
On 2016/09/09 01:34:14, diegocarloslima wrote:
> You might want to use Utils.getTag() to be consistent with other TAG variables
> that uses the simple name of the class

Acknowledged.

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:49:
}
On 2016/09/09 01:34:14, diegocarloslima wrote:
> I think that it would be more consistent if both of the methods
> 'generateUniqueFileName' returns a File instead of one returning a String and
> other a File. Also, since both would be always returning a File, it would make
> more sense to be renamed to 'generateUniqueFile'

Acknowledged.

https://codereview.adblockplus.org/29347192/diff/29349043/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:88:
}
On 2016/09/09 01:34:14, diegocarloslima wrote:
> Since none of these methods retains any state for the class instance, I think
> that it would make more sense to all be static methods

Acknowledged.

[diegocarloslima, 2016-11-03 14:06:03]

https://codereview.adblockplus.org/29347192/diff/29352560/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29352560/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:53:
public void delete(String filePath, boolean deleteSelf)
This method is missing the static modifier

[anton, 2016-12-02 06:15:03]

https://codereview.adblockplus.org/29347192/diff/29352560/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29352560/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:53:
public void delete(String filePath, boolean deleteSelf)
On 2016/11/03 14:06:02, diegocarloslima wrote:
> This method is missing the static modifier

Acknowledged.

[diegocarloslima, 2017-02-03 18:03:59]

On 2016/12/02 06:15:03, anton wrote:
>
https://codereview.adblockplus.org/29347192/diff/29352560/libadblockplus-andr...
> File
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
> (right):
> 
>
https://codereview.adblockplus.org/29347192/diff/29352560/libadblockplus-andr...
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:53:
> public void delete(String filePath, boolean deleteSelf)
> On 2016/11/03 14:06:02, diegocarloslima wrote:
> > This method is missing the static modifier
> 
> Acknowledged.

LGTM

[Felix Dahlke, 2017-05-09 08:06:46]

Sorry for the long delay - better late than never I guess :)

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:53:
public static void delete(String filePath, boolean deleteSelf)
I may have missed something, but it appears `deleteSelf` is never set to
anything but `true`. So maybe we should just assume that behaviour here? I'm not
a fan of having unused code paths.

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:29:
@Test
This looks like a mere indentation change - I'd prefer it if we could land this
as a separate commit first. Noissue is fine.

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/UpdateCheckTest.java
(right):

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/UpdateCheckTest.java:38:
protected String previousRequestUrl;
Looks like more indentation changes. As before, could we land this separately?
The same Noissue commit as the previous indentation fixes is fine.

[anton, 2017-05-10 11:00:14]

No need to continue with this code review, please see new one:
https://codereview.adblockplus.org/29435584/

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java
(right):

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/FileSystemUtils.java:53:
public static void delete(String filePath, boolean deleteSelf)
On 2017/05/09 08:06:45, Felix Dahlke wrote:
> I may have missed something, but it appears `deleteSelf` is never set to
> anything but `true`. So maybe we should just assume that behaviour here? I'm
not
> a fan of having unused code paths.

Done.

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java
(right):

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:29:
@Test
On 2017/05/09 08:06:45, Felix Dahlke wrote:
> This looks like a mere indentation change - I'd prefer it if we could land
this
> as a separate commit first. Noissue is fine.

Lot's of changes were done since that times, this file changes are already done.
See updated code review

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/UpdateCheckTest.java
(right):

https://codereview.adblockplus.org/29347192/diff/29366723/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/UpdateCheckTest.java:38:
protected String previousRequestUrl;
On 2017/05/09 08:06:45, Felix Dahlke wrote:
> Looks like more indentation changes. As before, could we land this separately?
> The same Noissue commit as the previous indentation fixes is fine.

Lot's of changes were done since that times, this file changes are already done.
See updated code review