[cleanup] Don't use virtual inheritance in gtest fixtures

and use Mixin pattern with linear inheritance instead. This will
allow to customize the way the Isolate is created.

Bug: v8:8238
Change-Id: Ic611df123653af3a0f2271394387492e440b5ea8
Reviewed-on: https://chromium-review.googlesource.com/c/1306433Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57106}

test/unittests/compiler/graph-unittest.cc

@@ -15,9 +15,7 @@ namespace internal {
 namespace compiler {
 
 GraphTest::GraphTest(int num_parameters)
-    : TestWithNativeContext(),
-      TestWithIsolateAndZone(),
-      canonical_(isolate()),
+    : canonical_(isolate()),
       common_(zone()),
       graph_(zone()),
       broker_(isolate(), zone()),

test/unittests/compiler/graph-unittest.h

@@ -24,8 +24,7 @@ namespace compiler {
 
 using ::testing::Matcher;
 
-class GraphTest : public virtual TestWithNativeContext,
-                  public virtual TestWithIsolateAndZone {
+class GraphTest : public TestWithNativeContextAndZone {
  public:
   explicit GraphTest(int num_parameters = 1);
   ~GraphTest() override;

test/unittests/compiler/instruction-selector-unittest.h

@@ -18,8 +18,7 @@ namespace v8 {
 namespace internal {
 namespace compiler {
 
-class InstructionSelectorTest : public TestWithContext,
-                                public TestWithIsolateAndZone {
+class InstructionSelectorTest : public TestWithNativeContextAndZone {
  public:
   InstructionSelectorTest();
   ~InstructionSelectorTest() override;

test/unittests/compiler/zone-stats-unittest.cc

@@ -10,7 +10,7 @@ namespace v8 {
 namespace internal {
 namespace compiler {
 
-class ZoneStatsTest : public TestWithIsolate {
+class ZoneStatsTest : public ::testing::Test {
  public:
   ZoneStatsTest() : zone_stats_(&allocator_) {}
 

test/unittests/test-utils.cc

@@ -15,109 +15,28 @@
 
 namespace v8 {
 
-// static
-v8::ArrayBuffer::Allocator* TestWithIsolate::array_buffer_allocator_ = nullptr;
-
-// static
-Isolate* TestWithIsolate::isolate_ = nullptr;
-
-TestWithIsolate::TestWithIsolate()
-    : isolate_scope_(isolate()), handle_scope_(isolate()) {}
-
-TestWithIsolate::~TestWithIsolate() = default;
-
-// static
-void TestWithIsolate::SetUpTestCase() {
-  Test::SetUpTestCase();
-  EXPECT_EQ(nullptr, isolate_);
+IsolateWrapper::IsolateWrapper()
+    : array_buffer_allocator_(
+          v8::ArrayBuffer::Allocator::NewDefaultAllocator()) {
   v8::Isolate::CreateParams create_params;
-  array_buffer_allocator_ = v8::ArrayBuffer::Allocator::NewDefaultAllocator();
   create_params.array_buffer_allocator = array_buffer_allocator_;
   isolate_ = v8::Isolate::New(create_params);
-  EXPECT_TRUE(isolate_ != nullptr);
+  CHECK_NOT_NULL(isolate_);
 }
 
-
-// static
-void TestWithIsolate::TearDownTestCase() {
-  ASSERT_TRUE(isolate_ != nullptr);
+IsolateWrapper::~IsolateWrapper() {
   v8::Platform* platform = internal::V8::GetCurrentPlatform();
-  ASSERT_TRUE(platform != nullptr);
+  CHECK_NOT_NULL(platform);
   while (platform::PumpMessageLoop(platform, isolate_)) continue;
   isolate_->Dispose();
-  isolate_ = nullptr;
   delete array_buffer_allocator_;
-  Test::TearDownTestCase();
-}
-
-Local<Value> TestWithIsolate::RunJS(const char* source) {
-  Local<Script> script =
-      v8::Script::Compile(
-          isolate()->GetCurrentContext(),
-          v8::String::NewFromUtf8(isolate(), source, v8::NewStringType::kNormal)
-              .ToLocalChecked())
-          .ToLocalChecked();
-  return script->Run(isolate()->GetCurrentContext()).ToLocalChecked();
-}
-
-Local<Value> TestWithIsolate::RunJS(
-    String::ExternalOneByteStringResource* source) {
-  Local<Script> script =
-      v8::Script::Compile(
-          isolate()->GetCurrentContext(),
-          v8::String::NewExternalOneByte(isolate(), source).ToLocalChecked())
-          .ToLocalChecked();
-  return script->Run(isolate()->GetCurrentContext()).ToLocalChecked();
-}
-
-TestWithContext::TestWithContext()
-    : context_(Context::New(isolate())), context_scope_(context_) {}
-
-TestWithContext::~TestWithContext() = default;
-
-v8::Local<v8::String> TestWithContext::NewString(const char* string) {
-  return v8::String::NewFromUtf8(v8_isolate(), string,
-                                 v8::NewStringType::kNormal)
-      .ToLocalChecked();
 }
 
-void TestWithContext::SetGlobalProperty(const char* name,
-                                        v8::Local<v8::Value> value) {
-  CHECK(v8_context()
-            ->Global()
-            ->Set(v8_context(), NewString(name), value)
-            .FromJust());
-}
+// static
+v8::IsolateWrapper* SharedIsolateHolder::isolate_wrapper_ = nullptr;
 
 namespace internal {
 
-TestWithIsolate::~TestWithIsolate() = default;
-
-TestWithIsolateAndZone::~TestWithIsolateAndZone() = default;
-
-Factory* TestWithIsolate::factory() const { return isolate()->factory(); }
-
-Handle<Object> TestWithIsolate::RunJSInternal(const char* source) {
-  return Utils::OpenHandle(*::v8::TestWithIsolate::RunJS(source));
-}
-
-Handle<Object> TestWithIsolate::RunJSInternal(
-    ::v8::String::ExternalOneByteStringResource* source) {
-  return Utils::OpenHandle(*::v8::TestWithIsolate::RunJS(source));
-}
-
-base::RandomNumberGenerator* TestWithIsolate::random_number_generator() const {
-  return isolate()->random_number_generator();
-}
-
-TestWithZone::~TestWithZone() = default;
-
-TestWithNativeContext::~TestWithNativeContext() = default;
-
-Handle<Context> TestWithNativeContext::native_context() const {
-  return isolate()->native_context();
-}
-
 SaveFlags::SaveFlags() { non_default_flags_ = FlagList::argv(); }
 
 SaveFlags::~SaveFlags() {

test/unittests/test-utils.h

@@ -21,126 +21,247 @@ namespace v8 {
 
 class ArrayBufferAllocator;
 
-// Use v8::internal::TestWithIsolate  if you are testing internals,
-// aka. directly work with Handles.
-class TestWithIsolate : public virtual ::testing::Test {
+// RAII-like Isolate instance wrapper.
+class IsolateWrapper final {
  public:
-  TestWithIsolate();
-  ~TestWithIsolate() override;
+  IsolateWrapper();
+  ~IsolateWrapper();
 
-  v8::Isolate* isolate() const { return v8_isolate(); }
+  v8::Isolate* isolate() const { return isolate_; }
 
-  v8::Isolate* v8_isolate() const { return isolate_; }
+ private:
+  v8::ArrayBuffer::Allocator* array_buffer_allocator_;
+  v8::Isolate* isolate_;
 
-  v8::internal::Isolate* i_isolate() const {
-    return reinterpret_cast<v8::internal::Isolate*>(isolate());
+  DISALLOW_COPY_AND_ASSIGN(IsolateWrapper);
+};
+
+class SharedIsolateHolder final {
+ public:
+  static v8::Isolate* isolate() { return isolate_wrapper_->isolate(); }
+
+  static void CreateIsolate() {
+    CHECK_NULL(isolate_wrapper_);
+    isolate_wrapper_ = new IsolateWrapper();
+  }
+
+  static void DeleteIsolate() {
+    CHECK_NOT_NULL(isolate_wrapper_);
+    delete isolate_wrapper_;
+    isolate_wrapper_ = nullptr;
+  }
+
+ private:
+  static v8::IsolateWrapper* isolate_wrapper_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(SharedIsolateHolder);
+};
+
+//
+// A set of mixins from which the test fixtures will be constructed.
+//
+template <typename TMixin>
+class WithSharedIsolateMixin : public TMixin {
+ public:
+  WithSharedIsolateMixin() = default;
+
+  v8::Isolate* v8_isolate() const { return SharedIsolateHolder::isolate(); }
+
+  static void SetUpTestCase() {
+    TMixin::SetUpTestCase();
+    SharedIsolateHolder::CreateIsolate();
+  }
+
+  static void TearDownTestCase() {
+    SharedIsolateHolder::DeleteIsolate();
+    TMixin::TearDownTestCase();
   }
 
-  Local<Value> RunJS(const char* source);
-  Local<Value> RunJS(String::ExternalOneByteStringResource* source);
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WithSharedIsolateMixin);
+};
+
+template <typename TMixin>
+class WithIsolateScopeMixin : public TMixin {
+ public:
+  WithIsolateScopeMixin()
+      : isolate_scope_(v8_isolate()), handle_scope_(v8_isolate()) {}
+
+  v8::Isolate* isolate() const { return v8_isolate(); }
+  v8::Isolate* v8_isolate() const { return TMixin::v8_isolate(); }
+
+  v8::internal::Isolate* i_isolate() const {
+    return reinterpret_cast<v8::internal::Isolate*>(v8_isolate());
+  }
 
-  static void SetUpTestCase();
-  static void TearDownTestCase();
+  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
+  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
 
  private:
-  static v8::ArrayBuffer::Allocator* array_buffer_allocator_;
-  static v8::Isolate* isolate_;
   v8::Isolate::Scope isolate_scope_;
   v8::HandleScope handle_scope_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestWithIsolate);
+  DISALLOW_COPY_AND_ASSIGN(WithIsolateScopeMixin);
 };
 
-// Use v8::internal::TestWithNativeContext if you are testing internals,
-// aka. directly work with Handles.
-class TestWithContext : public virtual v8::TestWithIsolate {
+template <typename TMixin>
+class WithContextMixin : public TMixin {
  public:
-  TestWithContext();
-  ~TestWithContext() override;
+  WithContextMixin()
+      : context_(Context::New(v8_isolate())), context_scope_(context_) {}
+
+  v8::Isolate* v8_isolate() const { return TMixin::v8_isolate(); }
 
   const Local<Context>& context() const { return v8_context(); }
   const Local<Context>& v8_context() const { return context_; }
 
-  v8::Local<v8::String> NewString(const char* string);
-  void SetGlobalProperty(const char* name, v8::Local<v8::Value> value);
+  Local<Value> RunJS(const char* source) {
+    return RunJS(v8::String::NewFromUtf8(v8_isolate(), source,
+                                         v8::NewStringType::kNormal)
+                     .ToLocalChecked());
+  }
+
+  Local<Value> RunJS(v8::String::ExternalOneByteStringResource* source) {
+    return RunJS(
+        v8::String::NewExternalOneByte(v8_isolate(), source).ToLocalChecked());
+  }
+
+  v8::Local<v8::String> NewString(const char* string) {
+    return v8::String::NewFromUtf8(v8_isolate(), string,
+                                   v8::NewStringType::kNormal)
+        .ToLocalChecked();
+  }
+
+  void SetGlobalProperty(const char* name, v8::Local<v8::Value> value) {
+    CHECK(v8_context()
+              ->Global()
+              ->Set(v8_context(), NewString(name), value)
+              .FromJust());
+  }
+
+  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
+  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
 
  private:
-  Local<Context> context_;
+  Local<Value> RunJS(Local<String> source) {
+    auto context = v8_isolate()->GetCurrentContext();
+    Local<Script> script =
+        v8::Script::Compile(context, source).ToLocalChecked();
+    return script->Run(context).ToLocalChecked();
+  }
+
+  v8::Local<v8::Context> context_;
   v8::Context::Scope context_scope_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestWithContext);
+  DISALLOW_COPY_AND_ASSIGN(WithContextMixin);
 };
 
+// Use v8::internal::TestWithIsolate if you are testing internals,
+// aka. directly work with Handles.
+using TestWithIsolate =          //
+    WithIsolateScopeMixin<       //
+        WithSharedIsolateMixin<  //
+            ::testing::Test>>;
+
+// Use v8::internal::TestWithNativeContext if you are testing internals,
+// aka. directly work with Handles.
+using TestWithContext =              //
+    WithContextMixin<                //
+        WithIsolateScopeMixin<       //
+            WithSharedIsolateMixin<  //
+                ::testing::Test>>>;
+
 namespace internal {
 
 // Forward declarations.
 class Factory;
 
-
-class TestWithIsolate : public virtual ::v8::TestWithIsolate {
+template <typename TMixin>
+class WithInternalIsolateMixin : public TMixin {
  public:
-  TestWithIsolate() = default;
-  ~TestWithIsolate() override;
+  WithInternalIsolateMixin() = default;
+
+  Factory* factory() const { return isolate()->factory(); }
+  Isolate* isolate() const { return TMixin::i_isolate(); }
+
+  Handle<Context> native_context() const { return isolate()->native_context(); }
 
-  Factory* factory() const;
-  Isolate* isolate() const { return i_isolate(); }
   template <typename T = Object>
   Handle<T> RunJS(const char* source) {
     return Handle<T>::cast(RunJSInternal(source));
   }
-  Handle<Object> RunJSInternal(const char* source);
+
+  Handle<Object> RunJSInternal(const char* source) {
+    return Utils::OpenHandle(*TMixin::RunJS(source));
+  }
+
   template <typename T = Object>
   Handle<T> RunJS(::v8::String::ExternalOneByteStringResource* source) {
     return Handle<T>::cast(RunJSInternal(source));
   }
-  Handle<Object> RunJSInternal(
-      ::v8::String::ExternalOneByteStringResource* source);
-
-  base::RandomNumberGenerator* random_number_generator() const;
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestWithIsolate);
-};
+  Handle<Object> RunJSInternal(
+      ::v8::String::ExternalOneByteStringResource* source) {
+    return Utils::OpenHandle(*TMixin::RunJS(source));
+  }
 
-class TestWithZone : public virtual ::testing::Test {
- public:
-  TestWithZone() : zone_(&allocator_, ZONE_NAME) {}
-  ~TestWithZone() override;
+  base::RandomNumberGenerator* random_number_generator() const {
+    return isolate()->random_number_generator();
+  }
 
-  Zone* zone() { return &zone_; }
+  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
+  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
 
  private:
-  v8::internal::AccountingAllocator allocator_;
-  Zone zone_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestWithZone);
+  DISALLOW_COPY_AND_ASSIGN(WithInternalIsolateMixin);
 };
 
-class TestWithIsolateAndZone : public virtual TestWithIsolate {
+template <typename TMixin>
+class WithZoneMixin : public TMixin {
  public:
-  TestWithIsolateAndZone() : zone_(&allocator_, ZONE_NAME) {}
-  ~TestWithIsolateAndZone() override;
+  WithZoneMixin() : zone_(&allocator_, ZONE_NAME) {}
 
   Zone* zone() { return &zone_; }
 
+  static void SetUpTestCase() { TMixin::SetUpTestCase(); }
+  static void TearDownTestCase() { TMixin::TearDownTestCase(); }
+
  private:
   v8::internal::AccountingAllocator allocator_;
   Zone zone_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestWithIsolateAndZone);
+  DISALLOW_COPY_AND_ASSIGN(WithZoneMixin);
 };
 
-class TestWithNativeContext : public virtual ::v8::TestWithContext,
-                              public virtual TestWithIsolate {
- public:
-  TestWithNativeContext() = default;
-  ~TestWithNativeContext() override;
-
-  Handle<Context> native_context() const;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestWithNativeContext);
-};
+using TestWithIsolate =              //
+    WithInternalIsolateMixin<        //
+        WithIsolateScopeMixin<       //
+            WithSharedIsolateMixin<  //
+                ::testing::Test>>>;
+
+using TestWithZone = WithZoneMixin<::testing::Test>;
+
+using TestWithIsolateAndZone =       //
+    WithInternalIsolateMixin<        //
+        WithIsolateScopeMixin<       //
+            WithSharedIsolateMixin<  //
+                WithZoneMixin<       //
+                    ::testing::Test>>>>;
+
+using TestWithNativeContext =            //
+    WithInternalIsolateMixin<            //
+        WithContextMixin<                //
+            WithIsolateScopeMixin<       //
+                WithSharedIsolateMixin<  //
+                    ::testing::Test>>>>;
+
+using TestWithNativeContextAndZone =         //
+    WithZoneMixin<                           //
+        WithInternalIsolateMixin<            //
+            WithContextMixin<                //
+                WithIsolateScopeMixin<       //
+                    WithSharedIsolateMixin<  //
+                        ::testing::Test>>>>>;
 
 class SaveFlags {
  public: