test: make IsolateData per-isolate in cctest

This ensures that we only create one IsolateData for each isolate inthe cctest, since IsolateData are meant to be per-isolate. We need to make the isolate and isolate_data static in the test fixtures as a result, similar to how the event loops and array buffer allocators are managed in the NodeZeroIsolateTestFixture but it is fine because gtest ensures that the Setup() and TearDown() of the fixtures are always run in order and would never overlap in one process. PR-URL: #48450 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
nodejs · Jul 3, 2023 · 30f4bc4 · 30f4bc4
1 parent 9750d82
commit 30f4bc4
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 29 deletions.
diff --git a/test/cctest/node_test_fixture.cc b/test/cctest/node_test_fixture.cc
@@ -6,7 +6,8 @@ uv_loop_t NodeZeroIsolateTestFixture::current_loop;
 NodePlatformUniquePtr NodeZeroIsolateTestFixture::platform;
 TracingAgentUniquePtr NodeZeroIsolateTestFixture::tracing_agent;
 bool NodeZeroIsolateTestFixture::node_initialized = false;
-
+v8::Isolate* NodeTestFixture::isolate_ = nullptr;
+node::IsolateData* EnvironmentTestFixture::isolate_data_ = nullptr;
 
 void NodeTestEnvironment::SetUp() {
   NodeZeroIsolateTestFixture::tracing_agent =

diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h
@@ -67,6 +67,7 @@ class NodeTestEnvironment final : public ::testing::Environment {
   void TearDown() override;
 };
 
+class NodeTestFixture;
 
 class NodeZeroIsolateTestFixture : public ::testing::Test {
  protected:
@@ -104,12 +105,13 @@ class NodeZeroIsolateTestFixture : public ::testing::Test {
   }
 
   friend NodeTestEnvironment;
+  friend NodeTestFixture;
 };
 
 
 class NodeTestFixture : public NodeZeroIsolateTestFixture {
  protected:
-  v8::Isolate* isolate_;
+  static v8::Isolate* isolate_;
 
   void SetUp() override {
     NodeZeroIsolateTestFixture::SetUp();
@@ -130,7 +132,22 @@ class NodeTestFixture : public NodeZeroIsolateTestFixture {
 
 
 class EnvironmentTestFixture : public NodeTestFixture {
- public:
+ protected:
+  static node::IsolateData* isolate_data_;
+
+  void SetUp() override {
+    NodeTestFixture::SetUp();
+    isolate_data_ = node::CreateIsolateData(NodeTestFixture::isolate_,
+                                            &NodeTestFixture::current_loop,
+                                            platform.get());
+    CHECK_NE(nullptr, isolate_data_);
+  }
+
+  void TearDown() override {
+    node::FreeIsolateData(isolate_data_);
+    NodeTestFixture::TearDown();
+  }
+
   class Env {
    public:
     Env(const v8::HandleScope& handle_scope,
@@ -142,23 +159,20 @@ class EnvironmentTestFixture : public NodeTestFixture {
       CHECK(!context_.IsEmpty());
       context_->Enter();
 
-      isolate_data_ = node::CreateIsolateData(isolate,
-                                              &NodeTestFixture::current_loop,
-                                              platform.get());
-      CHECK_NE(nullptr, isolate_data_);
       std::vector<std::string> args(*argv, *argv + 1);
       std::vector<std::string> exec_args(*argv, *argv + 1);
-      environment_ = node::CreateEnvironment(isolate_data_,
-                                             context_,
-                                             args,
-                                             exec_args,
-                                             flags);
+      DCHECK_EQ(EnvironmentTestFixture::isolate_data_->isolate(), isolate);
+      environment_ =
+          node::CreateEnvironment(EnvironmentTestFixture::isolate_data_,
+                                  context_,
+                                  args,
+                                  exec_args,
+                                  flags);
       CHECK_NE(nullptr, environment_);
     }
 
     ~Env() {
       node::FreeEnvironment(environment_);
-      node::FreeIsolateData(isolate_data_);
       context_->Exit();
     }
 
@@ -175,7 +189,6 @@ class EnvironmentTestFixture : public NodeTestFixture {
 
    private:
     v8::Local<v8::Context> context_;
-    node::IsolateData* isolate_data_;
     node::Environment* environment_;
   };
 };

diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
@@ -30,7 +30,7 @@ static std::string cb_1_arg;  // NOLINT(runtime/string)
 class EnvironmentTest : public EnvironmentTestFixture {
  private:
   void TearDown() override {
-    NodeTestFixture::TearDown();
+    EnvironmentTestFixture::TearDown();
     called_cb_1 = false;
     called_cb_2 = false;
     called_cb_ordered_1 = false;
@@ -674,12 +674,8 @@ TEST_F(EnvironmentTest, NestedMicrotaskQueue) {
       v8::String::NewFromUtf8Literal(isolate_, "mustCall"),
       must_call).Check();
 
-  node::IsolateData* isolate_data = node::CreateIsolateData(
-      isolate_, &NodeTestFixture::current_loop, platform.get());
-  CHECK_NE(nullptr, isolate_data);
-
-  node::Environment* env = node::CreateEnvironment(
-      isolate_data, context, {}, {});
+  node::Environment* env =
+      node::CreateEnvironment(isolate_data_, context, {}, {});
   CHECK_NE(nullptr, env);
 
   v8::Local<v8::Function> eval_in_env = node::LoadEnvironment(
@@ -718,7 +714,6 @@ TEST_F(EnvironmentTest, NestedMicrotaskQueue) {
   EXPECT_EQ(callback_calls, (IntVec { 1, 3, 6, 2, 4, 7, 5 }));
 
   node::FreeEnvironment(env);
-  node::FreeIsolateData(isolate_data);
 }
 
 static bool interrupted = false;
@@ -733,19 +728,15 @@ TEST_F(EnvironmentTest, RequestInterruptAtExit) {
   CHECK(!context.IsEmpty());
   context->Enter();
 
-  node::IsolateData* isolate_data = node::CreateIsolateData(
-      isolate_, &NodeTestFixture::current_loop, platform.get());
-  CHECK_NE(nullptr, isolate_data);
   std::vector<std::string> args(*argv, *argv + 1);
   std::vector<std::string> exec_args(*argv, *argv + 1);
   node::Environment* environment =
-      node::CreateEnvironment(isolate_data, context, args, exec_args);
+      node::CreateEnvironment(isolate_data_, context, args, exec_args);
   CHECK_NE(nullptr, environment);
 
   node::RequestInterrupt(environment, OnInterrupt, nullptr);
   node::FreeEnvironment(environment);
   EXPECT_TRUE(interrupted);
 
-  node::FreeIsolateData(isolate_data);
   context->Exit();
 }
diff --git a/test/cctest/test_node_api.cc b/test/cctest/test_node_api.cc
@@ -16,7 +16,7 @@ class NodeApiTest : public EnvironmentTestFixture {
  private:
   void SetUp() override { EnvironmentTestFixture::SetUp(); }
 
-  void TearDown() override { NodeTestFixture::TearDown(); }
+  void TearDown() override { EnvironmentTestFixture::TearDown(); }
 };
 
 TEST_F(NodeApiTest, CreateNodeApiEnv) {

diff --git a/test/cctest/test_report.cc b/test/cctest/test_report.cc
@@ -20,7 +20,7 @@ bool report_callback_called = false;
 class ReportTest : public EnvironmentTestFixture {
  private:
   void TearDown() override {
-    NodeTestFixture::TearDown();
+    EnvironmentTestFixture::TearDown();
     report_callback_called = false;
   }
 };