Issue 4031 - Implement tests for libadblockplus-android

Issue 4031 - Implement tests for libadblockplus-android

[anton, 2016-05-27 13:46:36]

Subjects to discuss:

1) AndroidWebRequest seems to be the only (working) inheritor of WebRequest and
it's copy-pasted from 'app' module. Discuss moving it to 'library' to avoid
copy-pastes.

2) Discuss moving ReferrerMapping class to 'library' from 'app' too as
https://hg.adblockplus.org/libadblockplus/file/tip/test/ReferrerMapping.cpp
tests it.

3) Some tests are not implemented because of missing abstractions (f.e.
FileSystem and Lazy.. impl) or missing gettetrs/settings in jni side and exists
in c++ code.

4) Some tests fail because of (most likely) bugs in jni (f.e. headers are not
mapped for WebRequest). According issue to be created.

[diegocarloslima, 2016-09-06 22:30:39]

https://codereview.adblockplus.org/29344967/diff/29349147/libadblockplus-andr...
File libadblockplus-android-tests/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29344967/diff/29349147/libadblockplus-andr...
libadblockplus-android-tests/AndroidManifest.xml:5: android:versionName="1.3" >
The versionCode/versionName for tests should follow the lib versioning, so we
should do a fresh start here too, as discussed in Issue 4030.

[diegocarloslima, 2016-09-08 23:09:54]

On 2016/09/06 22:30:39, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29344967/diff/29349147/libadblockplus-andr...
> File libadblockplus-android-tests/AndroidManifest.xml (right):
> 
>
https://codereview.adblockplus.org/29344967/diff/29349147/libadblockplus-andr...
> libadblockplus-android-tests/AndroidManifest.xml:5: android:versionName="1.3"
>
> The versionCode/versionName for tests should follow the lib versioning, so we
> should do a fresh start here too, as discussed in Issue 4030.

LGTM

[Felix Dahlke, 2016-09-12 13:52:15]

Looks about right, I did a high-level reviews and found a couple of nits. With
those addressed, we can land this.

However, I find it depressing that we're actually duplicating the C++ test code
here. We just got to find a better way to do this... Can you create an issue for
that and CC me on it? (Just describing the problem suffices, we can come up with
the solution later.)

https://codereview.adblockplus.org/29344967/diff/29351815/README.md
File README.md (right):

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode65
README.md:65: In the 'libadblockplus-android-tests' run:
Nit: "In the 'libadblockplus-android-tests' directory"?

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode80
README.md:80: to run installed instrumentation tests app
Nit: Missing period? :D

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/AndroidManifest.xml:20: android:label="AdBlockPlus
JNI binding library tests">
Nit: The "B" in Adblock Plus shouldn't be capitalised.

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/build.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/build.xml:31: <!-- if sdk.dir was not set from one
of the property file, then
Nit: "one of the property files"?

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/MockLogSystem.java
(right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/MockLogSystem.java:20:
public class MockLogSystem extends LogSystem
This is a bit messed up, I guess you're using Git?

Git attempts to detect file changes automatically - and gets it wrong quite
often. This is one such case, it thinks this file is a "copy from
[...]/AdblockPlusException.java" (see the unified diff). IIRC there was some
command line option to suppress this behaviour (or at least require a higher
"similarity index"). Please use that next time. Or, even better: Just upload
patches from the Mercurial repo :) You'll need to get your changes in there
anyway in order to push them.

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

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:72:
jsEngine.evaluate("foo.responseHeaders['Content-Type'].substr(0,10)").asString());
Nit: Missing space before comma and `10`? Same thing again below.

[anton, 2016-09-13 05:49:09]

https://codereview.adblockplus.org/29344967/diff/29351815/README.md
File README.md (right):

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode65
README.md:65: In the 'libadblockplus-android-tests' run:
On 2016/09/12 13:52:12, Felix Dahlke wrote:
> Nit: "In the 'libadblockplus-android-tests' directory"?

Yes, it's "Library tests" README section and it's about the tests. We're having
"libadblockplus-android" directory with jni-glue files and
"libadblockplus-android-tests" with the tests. We need to run in
"libadblockplus-android-tests" to build both tests and dependencies (jni glue)
so it's not mistyping

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode80
README.md:80: to run installed instrumentation tests app
On 2016/09/12 13:52:12, Felix Dahlke wrote:
> Nit: Missing period? :D

did not catch that. it can be called "instrumentation tests app". it's an
application that has instrumented code and actually the whole app is tests

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/AndroidManifest.xml:20: android:label="AdBlockPlus
JNI binding library tests">
On 2016/09/12 13:52:13, Felix Dahlke wrote:
> Nit: The "B" in Adblock Plus shouldn't be capitalised.

Acknowledged.

[Felix Dahlke, 2016-09-13 08:30:15]

https://codereview.adblockplus.org/29344967/diff/29351815/README.md
File README.md (right):

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode65
README.md:65: In the 'libadblockplus-android-tests' run:
On 2016/09/13 05:49:08, anton wrote:
> On 2016/09/12 13:52:12, Felix Dahlke wrote:
> > Nit: "In the 'libadblockplus-android-tests' directory"?
> 
> Yes, it's "Library tests" README section and it's about the tests. We're
having
> "libadblockplus-android" directory with jni-glue files and
> "libadblockplus-android-tests" with the tests. We need to run in
> "libadblockplus-android-tests" to build both tests and dependencies (jni glue)
> so it's not mistyping

Just to clarify, what I meant here is that the sentence sounds weird this way:
"In the $directory_name run", so I suggested to write "In the $directory_name
directory run". You also say "In the 'libadblockplus-android-tests' directory
run" below, so this sentence here seemed like it's just missing a "directory".

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode80
README.md:80: to run installed instrumentation tests app
On 2016/09/13 05:49:08, anton wrote:
> On 2016/09/12 13:52:12, Felix Dahlke wrote:
> > Nit: Missing period? :D
> 
> did not catch that. it can be called "instrumentation tests app". it's an
> application that has instrumented code and actually the whole app is tests

I know, I'm just saying that there appears to be a period (".") missing at the
end of the sentence.

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/build.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/build.xml:31: <!-- if sdk.dir was not set from one
of the property file, then
On 2016/09/12 13:52:13, Felix Dahlke wrote:
> Nit: "one of the property files"?

What about this comment?

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

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:72:
jsEngine.evaluate("foo.responseHeaders['Content-Type'].substr(0,10)").asString());
On 2016/09/12 13:52:13, Felix Dahlke wrote:
> Nit: Missing space before comma and `10`? Same thing again below.

What about this comment?

[anton, 2016-09-13 10:44:38]

uploaded patch set #6

https://codereview.adblockplus.org/29344967/diff/29351815/README.md
File README.md (right):

https://codereview.adblockplus.org/29344967/diff/29351815/README.md#newcode65
README.md:65: In the 'libadblockplus-android-tests' run:
On 2016/09/13 08:30:13, Felix Dahlke wrote:
> On 2016/09/13 05:49:08, anton wrote:
> > On 2016/09/12 13:52:12, Felix Dahlke wrote:
> > > Nit: "In the 'libadblockplus-android-tests' directory"?
> > 
> > Yes, it's "Library tests" README section and it's about the tests. We're
> having
> > "libadblockplus-android" directory with jni-glue files and
> > "libadblockplus-android-tests" with the tests. We need to run in
> > "libadblockplus-android-tests" to build both tests and dependencies (jni
glue)
> > so it's not mistyping
> 
> Just to clarify, what I meant here is that the sentence sounds weird this way:
> "In the $directory_name run", so I suggested to write "In the $directory_name
> directory run". You also say "In the 'libadblockplus-android-tests' directory
> run" below, so this sentence here seemed like it's just missing a "directory".

Acknowledged.

[Felix Dahlke, 2016-09-13 10:51:45]

There are still two comments which you didn't seem to have noticed so far.

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/build.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/build.xml:31: <!-- if sdk.dir was not set from one
of the property file, then
On 2016/09/13 08:30:14, Felix Dahlke wrote:
> On 2016/09/12 13:52:13, Felix Dahlke wrote:
> > Nit: "one of the property files"?
> 
> What about this comment?

You don't seem to have noticed this comment, hm?

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

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:72:
jsEngine.evaluate("foo.responseHeaders['Content-Type'].substr(0,10)").asString());
On 2016/09/13 08:30:14, Felix Dahlke wrote:
> On 2016/09/12 13:52:13, Felix Dahlke wrote:
> > Nit: Missing space before comma and `10`? Same thing again below.
> 
> What about this comment?

You don't seem to have noticed this comment, hm?

[anton, 2016-09-13 11:00:23]

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

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java:72:
jsEngine.evaluate("foo.responseHeaders['Content-Type'].substr(0,10)").asString());
On 2016/09/13 10:51:44, Felix Dahlke wrote:
> On 2016/09/13 08:30:14, Felix Dahlke wrote:
> > On 2016/09/12 13:52:13, Felix Dahlke wrote:
> > > Nit: Missing space before comma and `10`? Same thing again below.
> > 
> > What about this comment?
> 
> You don't seem to have noticed this comment, hm?

My bad, sorry. Going to fix it right now

[Felix Dahlke, 2016-09-15 08:54:39]

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/build.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/build.xml:31: <!-- if sdk.dir was not set from one
of the property file, then
On 2016/09/13 10:51:44, Felix Dahlke wrote:
> On 2016/09/13 08:30:14, Felix Dahlke wrote:
> > On 2016/09/12 13:52:13, Felix Dahlke wrote:
> > > Nit: "one of the property files"?
> > 
> > What about this comment?
> 
> You don't seem to have noticed this comment, hm?

You still don't seem to have noticed this comment.

Can you make sure to address all comments one way or another? If you tackled
them you can just hit "Done", then the reviewer knows you've changed something.
If you don't agree with a comment, please respond.

[anton, 2016-09-15 10:09:34]

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/build.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/build.xml:31: <!-- if sdk.dir was not set from one
of the property file, then
On 2016/09/15 08:54:38, Felix Dahlke wrote:
> On 2016/09/13 10:51:44, Felix Dahlke wrote:
> > On 2016/09/13 08:30:14, Felix Dahlke wrote:
> > > On 2016/09/12 13:52:13, Felix Dahlke wrote:
> > > > Nit: "one of the property files"?
> > > 
> > > What about this comment?
> > 
> > You don't seem to have noticed this comment, hm?
> 
> You still don't seem to have noticed this comment.
> 
> Can you make sure to address all comments one way or another? If you tackled
> them you can just hit "Done", then the reviewer knows you've changed
something.
> If you don't agree with a comment, please respond.

Sorry for not replying to this.

There are few "*.properties" files where local variable can be set:
ant.properties and local.properties (see above).
sdk.dir can be declared in any of them, usually in local.properties. Everything
looks good for me here.

[Felix Dahlke, 2016-09-15 10:31:27]

Almost there :D

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
File libadblockplus-android-tests/build.xml (right):

https://codereview.adblockplus.org/29344967/diff/29351815/libadblockplus-andr...
libadblockplus-android-tests/build.xml:31: <!-- if sdk.dir was not set from one
of the property file, then
On 2016/09/15 10:09:33, anton wrote:
> On 2016/09/15 08:54:38, Felix Dahlke wrote:
> > On 2016/09/13 10:51:44, Felix Dahlke wrote:
> > > On 2016/09/13 08:30:14, Felix Dahlke wrote:
> > > > On 2016/09/12 13:52:13, Felix Dahlke wrote:
> > > > > Nit: "one of the property files"?
> > > > 
> > > > What about this comment?
> > > 
> > > You don't seem to have noticed this comment, hm?
> > 
> > You still don't seem to have noticed this comment.
> > 
> > Can you make sure to address all comments one way or another? If you tackled
> > them you can just hit "Done", then the reviewer knows you've changed
> something.
> > If you don't agree with a comment, please respond.
> 
> Sorry for not replying to this.
> 
> There are few "*.properties" files where local variable can be set:
> ant.properties and local.properties (see above).
> sdk.dir can be declared in any of them, usually in local.properties.
Everything
> looks good for me here.

I know, I was just complaining about the grammar: "one of the property file" ->
"one of the property files".

[Felix Dahlke, 2016-09-15 15:17:03]

LGTM!