Issue 4612 - enable AA on first run and make automatic adding of any subscription optional

test/FilterEngine.cpp

Index: test/FilterEngine.cpp
diff --git a/test/FilterEngine.cpp b/test/FilterEngine.cpp
index 05cecfa71a4a2422204edf3dfeb0e0a12de905b6..c56fdd89a7891384efe742676db95e8bef567cdc 100644
--- a/test/FilterEngine.cpp
+++ b/test/FilterEngine.cpp
@@ -16,7 +16,9 @@
  */
 
 #include "BaseJsTest.h"
+#include <thread>
 
+using namespace AdblockPlus;

[Felix Dahlke, 2016/11/22 17:37:16]

Nit: Empty line after this to be in line with the other files?

[sergei, 2016/11/23 14:20:50]

Done.

 namespace
 {
   typedef std::shared_ptr<AdblockPlus::FilterEngine> FilterEnginePtr;
@@ -110,6 +112,50 @@ namespace
 
   // Workaround for https://issues.adblockplus.org/ticket/1397.
   void NoOpUpdaterCallback(const std::string&) {}
+
+  class FilterEngineWithFreshFolder : public ::testing::Test
+  {
+  protected:
+    FileSystemPtr m_fileSystem;

[Felix Dahlke, 2016/11/22 17:37:17]

From https://adblockplus.org/coding-style: "No hungarian notation, no special
variable name prefixes or suffixes denoting type or scope."

[sergei, 2016/11/23 14:20:50]

I thought we finally started to use #pragma once and prefixes for members of
classes in C++.
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h

This two things give a lot of help.

[Felix Dahlke, 2016/12/02 12:26:34]

#pragma is not restricted by the code style and we've started to use it at least
in adblockplusie (plus, I don't understand how that is related). Hungarian
notation is not something we started using to my knowledge (or should use unless
there's a really good reason IMO). But since you addressed it, we can discuss
that elsewhere (and maybe it is time to disconnect our C++ coding style from
Mozilla's, to make it easier to understand and evolve).

+    std::weak_ptr<JsEngine> m_jsEngine;
+
+    void SetUp() override
+    {
+      m_fileSystem.reset(new DefaultFileSystem());
+      // Since there are neither in memory FS nor functionality to work with

[Felix Dahlke, 2016/11/22 17:37:16]

Wording nit: "Since there 'is' neither 'in-memory' FS nor functionality to work
with directories',' [...]"

[sergei, 2016/11/23 14:20:50]

Done.

+      // directories use the hack: manually clean the directory.

[Felix Dahlke, 2016/11/22 17:37:16]

Do you mean "functionality to work with _a different directory_", maybe?

[sergei, 2016/11/23 14:20:50]

No, it's possible to configure working directory of DefaultFileSystem, I mean a
cross platform API to work with directories, for instance it would be useful to
have a scoped temporary directory or at least some functions to get a path to
temporary writable directory, create and remove directories and in addition
generate a temporary folder name. I have not added it here because I'm sure it
should be in another commit.

[Felix Dahlke, 2016/12/02 12:26:34]

Acknowledged.

+      removeFileIfExists("patterns.ini");
+      removeFileIfExists("prefs.json");
+    }
+    JsEnginePtr createJsEngine(const AppInfo& appInfo = AppInfo())
+    {
+      auto jsEngine = JsEngine::New(appInfo);

[Felix Dahlke, 2016/11/22 17:37:16]

I guess my C++ might be getting rusty, but why assign this to a temporary local
variable here, rather than directly to the member variable?

[sergei, 2016/11/23 14:20:50]

JsEngine::New returns std::shared_ptr but the type of member is std::weak_ptr,
it's a part of the hack with sleep_for used in removeFileIfExists. It is to deal
with the following race condition.

Currently we have issues with circular references
https://issues.adblockplus.org/ticket/3594 and with detached threads
https://issues.adblockplus.org/ticket/3595. Current version at master allows
different tests to influence another tests, for example, when the next test is
running, writing thread from a previous test can corrupt, e.g. patterns.ini.
Eliminating of circular references relieves a bit but not completely.
Asynchronous operation will be ignored if there is already no JsEngine, however
there is still a race condition, thread of asynchronous operation still can keep
JsEngine alive even when we have already no strong references to it. sleep_for
gives an asynchronous operation some time to finish its work, so afterwards we
can remove files created by current test. I have complicated the code even more
:) because it should take into account not only jsEngine of current test but
also async operations from previous tests (actually it happens too often on my
machine that patterns.ini is blocked by some previous test).

All these hacks will go away as the life span of asynchronous operations is no
longer than the life span of both JsEngine and FilterEngine.

[Felix Dahlke, 2016/12/02 12:26:34]

Acknowledged.

+      m_jsEngine = jsEngine;
+      jsEngine->SetFileSystem(m_fileSystem);
+      jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(new LazyWebRequest()));
+      jsEngine->SetLogSystem(AdblockPlus::LogSystemPtr(new LazyLogSystem()));
+      return jsEngine;
+    }
+    void TearDown() override
+    {
+      removeFileIfExists("patterns.ini");
+      removeFileIfExists("prefs.json");
+      m_fileSystem.reset();
+    }
+    void removeFileIfExists(const std::string& path)
+    {
+      {
+        // Hack: allow IO to finish currently running operations, in particular
+        // writing into files. Otherwise we get Permission denied.

[Felix Dahlke, 2016/11/22 17:37:16]

Wording nit: Maybe put "Permission denied" in quotes, or use a lower-case "p"?

[sergei, 2016/11/23 14:20:50]

Done.

+        int i = 100;
+        while (i-- > 0 && m_jsEngine.lock()) {

[Felix Dahlke, 2016/11/22 17:37:16]

Nit: Opening brace on its own line please (might as well skip the braces here of
course since there's just one statement in the body).

[sergei, 2016/11/23 14:20:50]

Done.

+          std::this_thread::sleep_for(std::chrono::seconds(2));
+        }
+      }
+      if (m_fileSystem->Stat(path).exists)
+        m_fileSystem->Remove(path);
+    }
+  };
 }
 
 TEST_F(FilterEngineTest, FilterCreation)
@@ -546,3 +592,40 @@ TEST(NewFilterEngineTest, MemoryLeak_NoCircularReferences)
   }
   EXPECT_FALSE(weakJsEngine.lock());
 }
+
+TEST_F(FilterEngineWithFreshFolder, LangAndAASubscriptionsAreChosenOnFirstRun)
+{
+  AppInfo appInfo;
+  appInfo.locale = "zh";
+  const std::string langSubscriptionUrl = "https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt";
+  auto jsEngine = createJsEngine(appInfo);
+  auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine));
+  const auto subscriptions = filterEngine->GetListedSubscriptions();
+  ASSERT_EQ(2u, subscriptions.size());
+  const auto aaUrl = filterEngine->GetPref("subscriptions_exceptionsurl")->AsString();

[Felix Dahlke, 2016/11/22 17:37:16]

Hm, maybe we should hard code the URL as well here, same as with the Chinese
subscription? No strong opinion, up to you.

[sergei, 2016/11/23 14:20:50]

I will remove aaUrl and use Subscription::IsAA instead in an upcoming commit.

[Felix Dahlke, 2016/12/02 12:26:34]

Acknowledged.

+  SubscriptionPtr aaSubscription;
+  SubscriptionPtr langSubscription;
+  if (subscriptions[0]->GetProperty("url")->AsString() == aaUrl)
+  {
+    aaSubscription = subscriptions[0];
+    langSubscription = subscriptions[1];
+  } else if (subscriptions[1]->GetProperty("url")->AsString() == aaUrl)

[Felix Dahlke, 2016/11/22 17:37:16]

Nit: Closing brace on its own line please.

[sergei, 2016/11/23 14:20:50]

Done.

+  {
+    aaSubscription = subscriptions[1];
+    langSubscription = subscriptions[0];
+  }
+  ASSERT_NE(nullptr, aaSubscription);
+  ASSERT_NE(nullptr, langSubscription);
+  EXPECT_EQ(aaUrl, aaSubscription->GetProperty("url")->AsString());
+  EXPECT_EQ(langSubscriptionUrl, langSubscription->GetProperty("url")->AsString());
+}
+
+TEST_F(FilterEngineWithFreshFolder, DisableSubscriptionsAutoSelectOnFirstRun)
+{
+  auto jsEngine = createJsEngine();
+  FilterEngine::Prefs preSettings;
+  preSettings["first_run_subscription_auto_select"] = jsEngine->NewValue(false);
+  auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine, preSettings));
+  const auto subscriptions = filterEngine->GetListedSubscriptions();
+  EXPECT_EQ(0u, subscriptions.size());
+}