Fix libadblockplus-android tests

adblock-android-tests/src/org/adblockplus/libadblockplus/BaseTest.java

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-present 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package org.adblockplus.libadblockplus;
+
+import android.content.Context;
+import android.os.SystemClock;
+import android.test.InstrumentationTestCase;
+
+import org.adblockplus.libadblockplus.AppInfo;
+import org.adblockplus.libadblockplus.FilterEngine;
+import org.adblockplus.libadblockplus.IsAllowedConnectionCallback;
+import org.adblockplus.libadblockplus.JsEngine;
+import org.adblockplus.libadblockplus.LazyLogSystem;
+import org.adblockplus.libadblockplus.LogSystem;
+import org.adblockplus.libadblockplus.Platform;
+import org.adblockplus.libadblockplus.ThrowingWebRequest;
+import org.adblockplus.libadblockplus.WebRequest;
+
+import java.io.File;
+
+public class BaseTest extends InstrumentationTestCase
+{
+  protected Platform platform;
+  protected JsEngine jsEngine;
+  protected FilterEngine filterEngine;
+  private final SetupInfo setupInfo;
+
+  public BaseTest()
+  {
+    super();
+    setupInfo = new SetupInfo();
+  }
+
+  @Override
+  protected void setUp() throws Exception
+  {
+    deleteFiles();
+    super.setUp();
+  }
+
+  private static class SetupInfo
+  {
+    public AppInfo appInfo;
+    public LogSystem logSystem;
+    public WebRequest webRequest;
+    public String basePath;
+    public IsAllowedConnectionCallback isAllowedConnectionCallback;
+
+    public SetupInfo()
+    {
+      appInfo = AppInfo.builder().build();
+      logSystem = new LazyLogSystem();
+      webRequest = new ThrowingWebRequest();
+    }
+
+    public void updateBasePath(Context context)
+    {
+      if (basePath == null)
+      {
+        basePath = context.getFilesDir().getAbsolutePath();
+      }
+    }
+  }
+
+  protected void setAppInfo(AppInfo appInfo)
+    {

[anton, 2018/08/24 12:25:03]

wrong indentation

[René Jeschke, 2018/08/27 20:02:15]

Done.

+        setupInfo.appInfo = appInfo;
+    }
+
+  protected void setLogSystem(LogSystem logSystem)
+    {

[anton, 2018/08/24 12:25:03]

wrong indentation

[René Jeschke, 2018/08/27 20:02:15]

Done.

+        setupInfo.logSystem = logSystem;
+    }
+
+  protected void setWebRequest(WebRequest webRequest)
+    {

[anton, 2018/08/24 12:25:04]

wrong indentation

[René Jeschke, 2018/08/27 20:02:15]

Done.

+        setupInfo.webRequest = webRequest;
+    }
+
+  protected void setBasePath(String basePath)
+    {

[anton, 2018/08/24 12:25:03]

wrong indentation

[René Jeschke, 2018/08/27 20:02:15]

Done.

+        setupInfo.basePath = basePath;
+    }
+
+  protected void setIsAllowedConnectionCallback(IsAllowedConnectionCallback callback)
+  {
+    setupInfo.isAllowedConnectionCallback = callback;
+  }
+
+  protected void deleteFiles()

[anton, 2018/08/24 12:25:03]

let's move it to `Util` or `Helper` class

[René Jeschke, 2018/08/27 20:02:15]

It is only used here and in the context of tests. Do we gain anything if we move
this method to a helper class?

+  {
+    for (File f : getContext().getFilesDir().listFiles())
+    {
+      f.delete();
+    }
+  }
+
+  protected void setupPlatform()
+  {
+    if (platform == null)
+    {
+      setupInfo.updateBasePath(getContext());
+      platform = new Platform(setupInfo.logSystem, setupInfo.webRequest, setupInfo.basePath);
+    }
+  }
+
+  protected void setupJsEngine()
+  {
+    setupPlatform();
+    if (jsEngine == null)
+    {
+      platform.setUpJsEngine(setupInfo.appInfo);
+      jsEngine = platform.getJsEngine();
+    }
+  }
+
+  protected void setupFilterEngine()
+  {
+    setupJsEngine();
+    if (filterEngine == null)
+    {
+      if (setupInfo.isAllowedConnectionCallback != null)
+      {
+        platform.setUpFilterEngine(setupInfo.isAllowedConnectionCallback);
+      }
+
+      filterEngine = platform.getFilterEngine();
+
+      if (filterEngine.isFirstRun())
+      {
+        // Wait until stuff got persisted ...
+        File patterns = new File(getContext().getFilesDir(), "patterns.ini");

[anton, 2018/08/24 12:25:04]

It sounds like we know too much of `libadblockplus` implementation details.
Let's say filename will be changed one day and the test fails, though it should
not as everything still works.

How do you think?

[René Jeschke, 2018/08/27 20:02:15]

You got a point, but libadblockplus does not implement any kind of event
callback system for getting any information about it's state. There are tests
that depend on a clean state. The solution you used before was using 'sleep(n)',
but this had two issues: any timeout you specify can be too short, it makes the
test use an unbearable amount of time to complete. Going back to the previous
state would mean getting back to unreliable tests which may fail without any
(real) reason.

If you know any other reliable way to query libadblockplus' persistence state,
then I will gladly remove this. Until then, this method provides a reliable way
to make the tests deterministic. Also, a file name change is more than unlikely,
as the file name is in valid 8.3 format and so supported by any file system that
I am aware of.

[René Jeschke, 2018/08/27 20:16:36]

As you said that everything 'still works', did you test it? On an emulator or a
real device? Here's what I get when I remove this check and run the tests on an
emulator:

org.adblockplus.libadblockplus.tests.AndroidWebRequestResourceWrapperTest >
testNotIntercepted_Easylist[Nexus_5X_API_27_x86(AVD) - 8.1.0] FAILED 
        junit.framework.AssertionFailedError: expected:<1> but was:<2>
        at
org.adblockplus.libadblockplus.tests.AndroidWebRequestResourceWrapperTest.testNotIntercepted(AndroidWebRequestResourceWrapperTest.java:241)

org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest >
testFirstRunClearWithData[Nexus_5X_API_27_x86(AVD) - 8.1.0] FAILED 
        junit.framework.AssertionFailedError
        at
org.adblockplus.libadblockplus.tests.FilterEngineFirstRunTest.testFirstRunClearWithData(FilterEngineFirstRunTest.java:48)

FAILURE: Build failed with an exception.

+        while (!patterns.exists())
+        {
+          SystemClock.sleep(50);
+        }
+      }
+    }
+  }
+
+  protected void disposeEngines()
+  {
+    filterEngine = null;
+    jsEngine = null;
+    if (platform != null)
+    {
+      platform.dispose();
+      platform = null;
+    }
+  }
+
+  protected Context getContext()
+  {
+    return getInstrumentation().getTargetContext();
+  }
+
+  @Override
+  protected void tearDown() throws Exception
+  {
+    super.tearDown();
+    disposeEngines();
+  }
+}