From f0363f5cf0645c59b928f6b94b75d5cd733cfb2e Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 26 Feb 2024 07:06:02 -0800 Subject: [PATCH] Enable varying feature flags in ReactInstanceIntegrationTest, use modern CDP registry by default (#43101) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/43101 Extends `ReactInstanceIntegrationTest` to allow varying feature flags in tests, using gtest's parameterised tests. Exercise this in `ConsoleLogTest` to test against the modern CDP registry, for which we also needed to modify some initialisation logic to account for the fact that under the modern registry, the page is added by the host, rather than by Hermes `DecoratedRuntime`. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D53919148 fbshipit-source-id: 4eb87abf548f30b5483b819a2dadd444d1d5c80d --- .../jsinspector-modern/InspectorFlags.cpp | 4 + .../jsinspector-modern/InspectorFlags.h | 12 +- .../tests/ReactInstanceIntegrationTest.cpp | 144 +++++++++++++++--- .../tests/ReactInstanceIntegrationTest.h | 26 +++- 4 files changed, 161 insertions(+), 25 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp index b0e3b4fb17b..4a2c9df723a 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.cpp @@ -37,6 +37,10 @@ bool InspectorFlags::getEnableCxxInspectorPackagerConnection() const { enableModernCDPRegistry_; } +void InspectorFlags::dangerouslyResetFlags() { + *this = InspectorFlags{}; +} + void InspectorFlags::assertFlagsMatchUpstream() const { if (inconsistentFlagsStateLogged_) { return; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h index 605fad31723..74ec341be42 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorFlags.h @@ -30,14 +30,20 @@ class InspectorFlags { */ bool getEnableCxxInspectorPackagerConnection() const; + /** + * Reset flags to their upstream values. The caller must ensure any resources + * that have read previous flag values have been cleaned up. + */ + void dangerouslyResetFlags(); + private: InspectorFlags(); InspectorFlags(const InspectorFlags&) = delete; - InspectorFlags& operator=(const InspectorFlags&) = delete; + InspectorFlags& operator=(const InspectorFlags&) = default; ~InspectorFlags() = default; - const bool enableModernCDPRegistry_; - const bool enableCxxInspectorPackagerConnection_; + bool enableModernCDPRegistry_; + bool enableCxxInspectorPackagerConnection_; mutable bool inconsistentFlagsStateLogged_; void assertFlagsMatchUpstream() const; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp index 9a1b07c207a..e305713edba 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp @@ -12,19 +12,53 @@ #include #include +#include +#include +#include #include namespace facebook::react::jsinspector_modern { using namespace ::testing; +class ReactInstanceIntegrationTestFeatureFlagsProvider + : public ReactNativeFeatureFlagsDefaults { + private: + FeatureFlags flags_; + + public: + explicit ReactInstanceIntegrationTestFeatureFlagsProvider( + FeatureFlags featureFlags) + : flags_(featureFlags) {} + + bool inspectorEnableModernCDPRegistry() override { + return flags_.enableModernCDPRegistry; + } + bool inspectorEnableCxxInspectorPackagerConnection() override { + return flags_.enableCxxInspectorPackagerConnection; + } +}; + ReactInstanceIntegrationTest::ReactInstanceIntegrationTest() : runtime(nullptr), instance(nullptr), messageQueueThread(std::make_shared()), - errorHandler(std::make_shared()) {} + errorHandler(std::make_shared()), + featureFlags(std::make_unique()) {} void ReactInstanceIntegrationTest::SetUp() { + ReactNativeFeatureFlags::override( + std::make_unique( + *featureFlags)); + // Reset InspectorFlags to overridden upstream values before creating objects + // that may access them. + InspectorFlags::getInstance().dangerouslyResetFlags(); + + // Double check that a flag matches our overrides. + ASSERT_EQ( + InspectorFlags::getInstance().getEnableModernCDPRegistry(), + featureFlags->enableModernCDPRegistry); + auto mockRegistry = std::make_unique(); auto timerManager = std::make_shared(std::move(mockRegistry)); @@ -47,11 +81,24 @@ void ReactInstanceIntegrationTest::SetUp() { "ErrorUtils", jsi::Object::createFromHostObject(*jsiRuntime, errorHandler)); + std::shared_ptr hostTargetIfModernCDP = nullptr; + + if (featureFlags->enableModernCDPRegistry) { + VoidExecutor inspectorExecutor = [this](auto callback) { + immediateExecutor_.add(callback); + }; + MockHostTargetDelegate hostTargetDelegate; + hostTargetIfModernCDP = + HostTarget::create(hostTargetDelegate, inspectorExecutor); + } + instance = std::make_unique( std::move(runtime_), messageQueueThread, timerManager, - std::move(jsErrorHandlingFunc)); + std::move(jsErrorHandlingFunc), + hostTargetIfModernCDP == nullptr ? nullptr : hostTargetIfModernCDP.get()); + timerManager->setRuntimeExecutor(instance->getBufferedRuntimeExecutor()); // JS Environment: @@ -59,18 +106,53 @@ void ReactInstanceIntegrationTest::SetUp() { // Inspector: auto& inspector = getInspectorInstance(); - auto pages = inspector.getPages(); - // We should now have at least a single page once the above runtime has been - // initialized. - assert(pages.size() > 0); - size_t pageId = pages.back().id; + if (hostTargetIfModernCDP != nullptr) { + // Under modern CDP, the React host is responsible for adding itself as + // the root target on startup. + pageId_ = inspector.addPage( + "mock-title", + "mock-vm", + [hostTargetIfModernCDP](std::unique_ptr remote) + -> std::unique_ptr { + auto localConnection = hostTargetIfModernCDP->connect( + std::move(remote), + { + .integrationName = "ReactInstanceIntegrationTest", + }); + return localConnection; + }, + // TODO: Allow customisation of InspectorTargetCapabilities + {}); + } else { + // Under legacy CDP, Hermes' DecoratedRuntime adds its page automatically + // within ConnectionDemux.enableDebugging. + auto pages = inspector.getPages(); + ASSERT_GT(pages.size(), 0); + pageId_ = pages.back().id; + } - clientToVM_ = inspector.connect(pageId, mockRemoteConnections_.make_unique()); + clientToVM_ = + inspector.connect(pageId_.value(), mockRemoteConnections_.make_unique()); + + ASSERT_NE(clientToVM_, nullptr); } void ReactInstanceIntegrationTest::TearDown() { clientToVM_->disconnect(); + // Destroy the local connection. + clientToVM_.reset(); + + if (pageId_.has_value() && featureFlags->enableModernCDPRegistry) { + // Under modern CDP, clean up the page we added in SetUp and destroy + // resources owned by HostTarget. + getInspectorInstance().removePage(pageId_.value()); + } + pageId_.reset(); + + // Expect the remote connection to have been destroyed. + EXPECT_EQ(mockRemoteConnections_[0], nullptr); + ReactNativeFeatureFlags::dangerouslyReset(); } void ReactInstanceIntegrationTest::initializeRuntime(std::string_view script) { @@ -84,8 +166,6 @@ void ReactInstanceIntegrationTest::initializeRuntime(std::string_view script) { std::string init(script); // JS calls no longer buffered after calling loadScript instance->loadScript(std::make_unique(init), ""); - - messageQueueThread->flush(); } void ReactInstanceIntegrationTest::send( @@ -134,18 +214,28 @@ TEST_F(ReactInstanceIntegrationTest, RuntimeEvalTest) { EXPECT_EQ(val.asNumber(), 3); } -TEST_F(ReactInstanceIntegrationTest, ConsoleLogTest) { - InSequence s; - - EXPECT_CALL(getRemoteConnection(), onMessage(_)) - .Times(2) - .RetiresOnSaturation(); - +TEST_P(ReactInstanceIntegrationTestWithFlags, ConsoleLog) { EXPECT_CALL( getRemoteConnection(), - onMessage(JsonParsed(AllOf( - AtJsonPtr("/params/args/0/value", Eq("Hello, World!")), - AtJsonPtr("/method", Eq("Runtime.consoleAPICalled")))))); + onMessage(JsonParsed( + AtJsonPtr("/method", Eq("Runtime.executionContextCreated"))))); + + EXPECT_CALL( + getRemoteConnection(), onMessage(JsonParsed(AtJsonPtr("/id", Eq(1))))); + + InSequence s; + + // Hermes console.* interception is currently explicitly disabled under the + // modern registry, and the runtime does not yet fire these events. When the + // implementation is more complete we should be able to remove this + // condition. + if (!featureFlags->enableModernCDPRegistry) { + EXPECT_CALL( + getRemoteConnection(), + onMessage(JsonParsed(AllOf( + AtJsonPtr("/params/args/0/value", Eq("Hello, World!")), + AtJsonPtr("/method", Eq("Runtime.consoleAPICalled")))))); + } EXPECT_CALL(getRemoteConnection(), onDisconnect()); @@ -153,4 +243,18 @@ TEST_F(ReactInstanceIntegrationTest, ConsoleLogTest) { run("console.log('Hello, World!');"); } +INSTANTIATE_TEST_SUITE_P( + ReactInstanceVaryingInspectorFlags, + ReactInstanceIntegrationTestWithFlags, + ::testing::Values( + FeatureFlags{ + .enableCxxInspectorPackagerConnection = true, + .enableModernCDPRegistry = true}, + FeatureFlags{ + .enableCxxInspectorPackagerConnection = false, + .enableModernCDPRegistry = false}, + FeatureFlags{ + .enableCxxInspectorPackagerConnection = true, + .enableModernCDPRegistry = false})); + } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.h b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.h index d21144bdbcb..b95500f609c 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.h @@ -7,6 +7,7 @@ #pragma once +#include #include #include #include @@ -18,7 +19,14 @@ namespace facebook::react::jsinspector_modern { -class ReactInstanceIntegrationTest : public ::testing::Test { +using namespace ::testing; + +struct FeatureFlags { + const bool enableCxxInspectorPackagerConnection = true; + const bool enableModernCDPRegistry = true; +}; + +class ReactInstanceIntegrationTest : public Test { protected: ReactInstanceIntegrationTest(); void SetUp() override; @@ -36,9 +44,13 @@ class ReactInstanceIntegrationTest : public ::testing::Test { std::unique_ptr instance; std::shared_ptr messageQueueThread; std::shared_ptr errorHandler; + std::unique_ptr featureFlags; MockRemoteConnection& getRemoteConnection() { - return *mockRemoteConnections_[0]; + EXPECT_EQ(mockRemoteConnections_.objectsVended(), 1); + auto rawPtr = mockRemoteConnections_[0]; + ASSERT(rawPtr != nullptr); + return *rawPtr; } private: @@ -46,8 +58,18 @@ class ReactInstanceIntegrationTest : public ::testing::Test { size_t id_ = 1; bool verbose_ = false; + std::optional pageId_; UniquePtrFactory mockRemoteConnections_; std::unique_ptr clientToVM_; + folly::QueuedImmediateExecutor immediateExecutor_; }; +class ReactInstanceIntegrationTestWithFlags + : public ReactInstanceIntegrationTest, + public ::testing::WithParamInterface { + void SetUp() override { + featureFlags = std::make_unique(GetParam()); + ReactInstanceIntegrationTest::SetUp(); + } +}; } // namespace facebook::react::jsinspector_modern