From 3ae96ae38082fcdbb915a0f2b548f68e2c627e14 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 25 Jun 2023 07:41:19 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/48450 Reviewed-By: Chengzhong Wu --- test/cctest/node_test_fixture.cc | 3 ++- test/cctest/node_test_fixture.h | 39 +++++++++++++++++++++----------- test/cctest/test_environment.cc | 17 ++++---------- test/cctest/test_node_api.cc | 2 +- test/cctest/test_report.cc | 2 +- 5 files changed, 34 insertions(+), 29 deletions(-) diff --git a/test/cctest/node_test_fixture.cc b/test/cctest/node_test_fixture.cc index 8179c7864436b1..eb39388a7462d9 100644 --- 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 index 4c687118a37054..3414c0be8ad777 100644 --- 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 args(*argv, *argv + 1); std::vector 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 context_; - node::IsolateData* isolate_data_; node::Environment* environment_; }; }; diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 547c8ddbffe243..09dcb1dccc1b28 100644 --- 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 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 args(*argv, *argv + 1); std::vector 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 index 8921b9d8d373db..902776c157e9ed 100644 --- 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 index 861fa40385e206..82c5c4e538dae4 100644 --- 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; } };