Enable varying feature flags in ReactInstanceIntegrationTest, use modern CDP registry by default (#43101)

Summary:
Pull Request resolved: #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

Author: robhogan

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;

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;

packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp

@@ -12,19 +12,53 @@
 
 #include <folly/json.h>
 #include <glog/logging.h>
+#include <jsinspector-modern/InspectorFlags.h>
+#include <react/featureflags/ReactNativeFeatureFlags.h>
+#include <react/featureflags/ReactNativeFeatureFlagsDefaults.h>
 #include <react/runtime/hermes/HermesInstance.h>
 
 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<MockMessageQueueThread>()),
-      errorHandler(std::make_shared<ErrorUtils>()) {}
+      errorHandler(std::make_shared<ErrorUtils>()),
+      featureFlags(std::make_unique<FeatureFlags>()) {}
 
 void ReactInstanceIntegrationTest::SetUp() {
+  ReactNativeFeatureFlags::override(
+      std::make_unique<ReactInstanceIntegrationTestFeatureFlagsProvider>(
+          *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<MockTimerRegistry>();
   auto timerManager =
       std::make_shared<react::TimerManager>(std::move(mockRegistry));
@@ -47,30 +81,78 @@ void ReactInstanceIntegrationTest::SetUp() {
       "ErrorUtils",
       jsi::Object::createFromHostObject(*jsiRuntime, errorHandler));
 
+  std::shared_ptr<HostTarget> hostTargetIfModernCDP = nullptr;
+
+  if (featureFlags->enableModernCDPRegistry) {
+    VoidExecutor inspectorExecutor = [this](auto callback) {
+      immediateExecutor_.add(callback);
+    };
+    MockHostTargetDelegate hostTargetDelegate;
+    hostTargetIfModernCDP =
+        HostTarget::create(hostTargetDelegate, inspectorExecutor);
+  }
+
   instance = std::make_unique<react::ReactInstance>(
       std::move(runtime_),
       messageQueueThread,
       timerManager,
-      std::move(jsErrorHandlingFunc));
+      std::move(jsErrorHandlingFunc),
+      hostTargetIfModernCDP == nullptr ? nullptr : hostTargetIfModernCDP.get());
+
   timerManager->setRuntimeExecutor(instance->getBufferedRuntimeExecutor());
 
   // JS Environment:
   initializeRuntime(preludeJsCode);
 
   // 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<IRemoteConnection> remote)
+            -> std::unique_ptr<ILocalConnection> {
+          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<react::JSBigStdString>(init), "");
-
-  messageQueueThread->flush();
 }
 
 void ReactInstanceIntegrationTest::send(
@@ -134,23 +214,47 @@ 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());
 
   send("Runtime.enable");
   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

packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.h

@@ -7,6 +7,7 @@
 
 #pragma once
 
+#include <folly/executors/QueuedImmediateExecutor.h>
 #include <folly/json.h>
 #include <gtest/gtest.h>
 #include <jsinspector-modern/InspectorInterfaces.h>
@@ -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,18 +44,32 @@ class ReactInstanceIntegrationTest : public ::testing::Test {
   std::unique_ptr<react::ReactInstance> instance;
   std::shared_ptr<MockMessageQueueThread> messageQueueThread;
   std::shared_ptr<ErrorUtils> errorHandler;
+  std::unique_ptr<FeatureFlags> featureFlags;
 
   MockRemoteConnection& getRemoteConnection() {
-    return *mockRemoteConnections_[0];
+    EXPECT_EQ(mockRemoteConnections_.objectsVended(), 1);
+    auto rawPtr = mockRemoteConnections_[0];
+    ASSERT(rawPtr != nullptr);
+    return *rawPtr;
   }
 
  private:
   void initializeRuntime(std::string_view script);
 
   size_t id_ = 1;
   bool verbose_ = false;
+  std::optional<int> pageId_;
   UniquePtrFactory<MockRemoteConnection> mockRemoteConnections_;
   std::unique_ptr<ILocalConnection> clientToVM_;
+  folly::QueuedImmediateExecutor immediateExecutor_;
 };
 
+class ReactInstanceIntegrationTestWithFlags
+    : public ReactInstanceIntegrationTest,
+      public ::testing::WithParamInterface<FeatureFlags> {
+  void SetUp() override {
+    featureFlags = std::make_unique<FeatureFlags>(GetParam());
+    ReactInstanceIntegrationTest::SetUp();
+  }
+};
 } // namespace facebook::react::jsinspector_modern