Issue 5136 - Add test for AndroidWebRequestResourceWrapper

Issue 5136 - Add test for AndroidWebRequestResourceWrapper

[anton, 2017-04-13 13:46:50]

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File libadblockplus-android-tests/build.gradle (right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/build.gradle:35: res.srcDirs = ['res']
now we need some preloaded subscriptions files in tests resources

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsTest.java:28:
public abstract class BaseJsTest extends InstrumentationTestCase
I had to change base class to more powerful as now we need access to test
resources (previously we were able to access only library resources and now we
need tests resources too). Otherwise we will be able to compile and runtime
exception (resoruce not found) will be thrown as it's in test resources not in
the library.

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsTest.java:43:
protected Context getContext()
for back compatibility for existing tests

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/BaseJsTest.java:45:
return getInstrumentation().getTargetContext();
target context = library context so it will work for existing tests just like
previously

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequestResourceWrapper.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequestResourceWrapper.java:115:
String urlWithoutParams = Utils.getUrlWithoutParams(url);
actually extracting url without params isn't a duty of this wrapper so i had to
refactor it a bit (move to Utils).
Added a tests for it too.

[anton, 2017-04-13 13:51:41]

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:121:
private void updateSubscriptions()
actually it's copy-paste from AdblockEngine but it would be too heavy to use
AdblockEngine instead of already existing FilterEngine. I could extract static
method with filterEngine argument like `updateFilterEngine(FilterEngine engine)`
but i think it does not worth it just for testing.

[diegocarloslima, 2017-04-27 18:57:47]

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:43:
private class TestRequest extends AndroidWebRequest
This inner class can be static

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:61:
private class TestStorage implements AndroidWebRequestResourceWrapper.Storage
This inner class can be static

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:83:
private class TestWrapperListener implements
AndroidWebRequestResourceWrapper.Listener
This inner class can be static

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:121:
private void updateSubscriptions()
On 2017/04/13 13:51:41, anton wrote:
> actually it's copy-paste from AdblockEngine but it would be too heavy to use
> AdblockEngine instead of already existing FilterEngine. I could extract static
> method with filterEngine argument like `updateFilterEngine(FilterEngine
engine)`
> but i think it does not worth it just for testing.

Yeah, since it's just for testing... I think you can keep like this for now

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:185:
public void testIntercepted_Easylist()
Why there's this underscore on the method name? I think you should stick to the
camelCase convention for method declaration

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:192:
public void testIntercepted_AcceptableAds()
Same here, I think you should stick to the camelCase convention for method
declaration

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:199:
public void testIntercepted_OnceOnly()
Same here, I think you should stick to the camelCase convention for method
declaration

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/UtilsTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/UtilsTest.java:57:
public void testUrlWithoutParams_null()
Same for the methods in this class, I think you should stick to the camelCase
convention for method declaration

[anton, 2017-04-28 06:04:49]

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:43:
private class TestRequest extends AndroidWebRequest
On 2017/04/27 18:57:46, diegocarloslima wrote:
> This inner class can be static

Acknowledged.
Also it can be final. Taking into account this is a test are these changes
really required?

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:61:
private class TestStorage implements AndroidWebRequestResourceWrapper.Storage
On 2017/04/27 18:57:46, diegocarloslima wrote:
> This inner class can be static

Acknowledged.

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:83:
private class TestWrapperListener implements
AndroidWebRequestResourceWrapper.Listener
On 2017/04/27 18:57:46, diegocarloslima wrote:
> This inner class can be static

Acknowledged.

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:121:
private void updateSubscriptions()
On 2017/04/27 18:57:46, diegocarloslima wrote:
> On 2017/04/13 13:51:41, anton wrote:
> > actually it's copy-paste from AdblockEngine but it would be too heavy to use
> > AdblockEngine instead of already existing FilterEngine. I could extract
static
> > method with filterEngine argument like `updateFilterEngine(FilterEngine
> engine)`
> > but i think it does not worth it just for testing.
> 
> Yeah, since it's just for testing... I think you can keep like this for now

Acknowledged.

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:185:
public void testIntercepted_Easylist()
On 2017/04/27 18:57:46, diegocarloslima wrote:
> Why there's this underscore on the method name? I think you should stick to
the
> camelCase convention for method declaration

"testIntercepted" is a general test direction (for successful result, see
another test methods) and underline separates it from what is actually checked.
See
https://developer.android.com/training/testing/unit-testing/instrumented-unit...
also uses it:

`public void logHistory_ParcelableWriteRead()`

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:192:
public void testIntercepted_AcceptableAds()
On 2017/04/27 18:57:47, diegocarloslima wrote:
> Same here, I think you should stick to the camelCase convention for method
> declaration

Replied above

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:199:
public void testIntercepted_OnceOnly()
On 2017/04/27 18:57:46, diegocarloslima wrote:
> Same here, I think you should stick to the camelCase convention for method
> declaration

Replied above

[anton, 2017-04-28 06:10:36]

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:43:
private class TestRequest extends AndroidWebRequest
On 2017/04/27 18:57:46, diegocarloslima wrote:
> This inner class can be static

uploaded new patch set

[diegocarloslima, 2017-04-28 12:53:58]

On 2017/04/28 06:10:36, anton wrote:
>
https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
> File
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java
> (right):
> 
>
https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
>
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:43:
> private class TestRequest extends AndroidWebRequest
> On 2017/04/27 18:57:46, diegocarloslima wrote:
> > This inner class can be static
> 
> uploaded new patch set

LGTM

[diegocarloslima, 2017-04-28 12:54:36]

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
File
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java
(right):

https://codereview.adblockplus.org/29411633/diff/29411634/libadblockplus-andr...
libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java:185:
public void testIntercepted_Easylist()
On 2017/04/28 06:04:48, anton wrote:
> On 2017/04/27 18:57:46, diegocarloslima wrote:
> > Why there's this underscore on the method name? I think you should stick to
> the
> > camelCase convention for method declaration
> 
> "testIntercepted" is a general test direction (for successful result, see
> another test methods) and underline separates it from what is actually
checked.
> See
>
https://developer.android.com/training/testing/unit-testing/instrumented-unit...
> also uses it:
> 
> `public void logHistory_ParcelableWriteRead()` 

Acknowledged.

[Felix Dahlke, 2017-05-09 08:10:49]

LGTM with the nit addressed.

https://codereview.adblockplus.org/29411633/diff/29424567/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/Utils.java
(right):

https://codereview.adblockplus.org/29411633/diff/29424567/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/Utils.java:142:
if (urlWithParams == null) {
Nit: Opening brace should go on its own line.

[anton, 2017-05-10 07:46:54]

https://codereview.adblockplus.org/29411633/diff/29424567/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/Utils.java
(right):

https://codereview.adblockplus.org/29411633/diff/29424567/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/Utils.java:142:
if (urlWithParams == null) {
On 2017/05/09 08:10:49, Felix Dahlke wrote:
> Nit: Opening brace should go on its own line.

Acknowledged.