Issue #3593, #1197- fix isolate management

test/BaseJsTest.cpp

Index: test/BaseJsTest.cpp
diff --git a/test/BaseJsTest.cpp b/test/BaseJsTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..5b864119bf43ca62673076fe2a15d9931b33778d
--- /dev/null
+++ b/test/BaseJsTest.cpp
@@ -0,0 +1,24 @@
+/*
+* This file is part of Adblock Plus <https://adblockplus.org/>,
+* Copyright (C) 2006-2015 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/>.
+*/
+
+#include "BaseJsTest.h"
+
+AdblockPlus::JsEnginePtr createJsEngine(const AdblockPlus::AppInfo& appInfo)
+{
+  static AdblockPlus::ScopedV8IsolatePtr isolate = std::make_shared<AdblockPlus::ScopedV8Isolate>();

[Eric, 2016/01/26 14:48:58]

What's the motivation for using a single isolate for the tests? I can imagine
that it's performance of the unit tests, but it doesn't say here.

It _does_, however, make a difference to the semantics of the tests. By using a
single isolate for all tests, it makes the tests interact with each other
through potential side effects that a test leaves on the engine. That doesn't
seem exactly desirable. If we're testing side effects, we should be doing so
explicitly rather than implicitly through some artifact of the test harness.

If performance is indeed the reason, and if it trumps isolation of the tests,
then that trade-off should be documented here. In other words, some code change
is needed here, even if it's only a comment.

[sergei, 2016/01/27 15:06:10]

Have you read the description of the codereview?

[Eric, 2016/01/27 17:21:02]

Yes. It wasn't clear. It's only slightly more clear in the most recent patch
set.

+  return AdblockPlus::JsEngine::New(appInfo, isolate);
+}