Issue 5136 - Add test for AndroidWebRequestResourceWrapper

libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestResourceWrapperTest.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 22 matching lines @@
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
 public class AndroidWebRequestResourceWrapperTest extends FilterEngineGenericTest
 {
   private static final int UPDATE_SUBSCRIPTIONS_WAIT_DELAY_MS = 5 * 1000; // 5s
 
-  private class TestRequest extends AndroidWebRequest

[diegocarloslima, 2017/04/27 18:57:46]

This inner class can be static

[anton, 2017/04/28 06:04:48]

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

[anton, 2017/04/28 06:10:35]

uploaded new patch set

+  private static final class TestRequest extends AndroidWebRequest
   {
     private List<String> urls = new LinkedList<String>();
 
     public List<String> getUrls()
     {
       return urls;
     }
 
     @Override
     public ServerResponse httpGET(String urlStr, List<HeaderEntry> headers)
     {
       urls.add(urlStr);
       return super.httpGET(urlStr, headers);
     }
   }
 
   // in-memory storage for testing only
-  private class TestStorage implements AndroidWebRequestResourceWrapper.Storage

[diegocarloslima, 2017/04/27 18:57:46]

This inner class can be static

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

Acknowledged.

+  private static final class TestStorage implements AndroidWebRequestResourceWrapper.Storage
   {
     private Set<String> interceptedUrls = new HashSet<String>();
 
     public Set<String> getInterceptedUrls()
     {
       return interceptedUrls;
     }
 
     @Override
     public synchronized void put(String url)
     {
       interceptedUrls.add(url);
     }
 
     @Override
     public synchronized boolean contains(String url)
     {
       return interceptedUrls.contains(url);
     }
   }
 
-  private class TestWrapperListener implements AndroidWebRequestResourceWrapper.Listener

[diegocarloslima, 2017/04/27 18:57:46]

This inner class can be static

[anton, 2017/04/28 06:04:48]

Acknowledged.

+  private static final class TestWrapperListener implements AndroidWebRequestResourceWrapper.Listener
   {
     private Map<String, Integer> urlsToResourceId = new HashMap<String, Integer>();
 
     public Map<String, Integer> getUrlsToResourceId()
     {
       return urlsToResourceId;
     }
 
     @Override
     public void onIntercepted(String url, int resourceId)
@@ skipping 17 matching lines @@
     preloadMap = new HashMap<String, Integer>();
     storage = new TestStorage();
     wrapper = new AndroidWebRequestResourceWrapper(
       getInstrumentation().getContext(), request, preloadMap, storage);
     wrapperListener = new TestWrapperListener();
     wrapper.setListener(wrapperListener);
 
     jsEngine.setWebRequest(wrapper);
   }
 
   private void updateSubscriptions()

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

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:46]

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

[anton, 2017/04/28 06:04:48]

Acknowledged.

   {
     for (final Subscription s : this.filterEngine.getListedSubscriptions())
     {
       try
       {
         s.updateFilters();
       }
       finally
       {
         s.dispose();
@@ skipping 43 matching lines @@
     {
       if (Utils.getUrlWithoutParams(eachString).equals(preloadUrl))
       {
         assertEquals(resourceId, wrapperListener.getUrlsToResourceId().get(eachString).intValue());
         break;
       }
     }
   }
 
   @Test
   public void testIntercepted_Easylist()

[diegocarloslima, 2017/04/27 18:57:46]

Why there's this underscore on the method name? I think you should stick to the
camelCase convention for method declaration

[anton, 2017/04/28 06:04:48]

"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()` 

[diegocarloslima, 2017/04/28 12:54:35]

Acknowledged.

   {
     testIntercepted(
       AndroidWebRequestResourceWrapper.EASYLIST, R.raw.easylist);
   }
 
   @Test
   public void testIntercepted_AcceptableAds()

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

Same here, I think you should stick to the camelCase convention for method
declaration

[anton, 2017/04/28 06:04:48]

Replied above

   {
     testIntercepted(
       AndroidWebRequestResourceWrapper.ACCEPTABLE_ADS, R.raw.exceptionrules);
   }
 
   @Test
   public void testIntercepted_OnceOnly()

[diegocarloslima, 2017/04/27 18:57:46]

Same here, I think you should stick to the camelCase convention for method
declaration

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

Replied above

   {
     final String preloadUrl = AndroidWebRequestResourceWrapper.EASYLIST;
 
     preloadMap.clear();
     preloadMap.put(preloadUrl, R.raw.easylist);
 
     assertEquals(0, request.getUrls().size());
 
     assertEquals(0, storage.getInterceptedUrls().size());
 
@@ skipping 113 matching lines @@
   }
 
   @Test
   public void testNotIntercepted_AcceptableAds()
   {
     testNotIntercepted(
       AndroidWebRequestResourceWrapper.EASYLIST, R.raw.easylist,
       AndroidWebRequestResourceWrapper.ACCEPTABLE_ADS);
   }
 }