libassistant-v2: Enable V2 APIs by default

Bug: b:170168612 Test: Added tests Change-Id: Ie3eb91bbcda09b6f6631ee01a318b385129250a6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263390 Reviewed-by: Yuki Awano <yawano@google.com> Commit-Queue: Tao Wu <wutao@chromium.org> Cr-Commit-Position: refs/heads/main@{#1107192}
chromium · Feb 18, 2023 · 0022f95 · 0022f95
1 parent d9dd583
commit 0022f95
Show file tree

Hide file tree

Showing 20 changed files with 267 additions and 40 deletions.
diff --git a/chrome/browser/ui/ash/assistant/assistant_browsertest.cc b/chrome/browser/ui/ash/assistant/assistant_browsertest.cc
@@ -55,8 +55,17 @@ class AssistantBrowserTest : public MixinBasedInProcessBrowserTest,
                              public testing::WithParamInterface<bool> {
  public:
   AssistantBrowserTest() {
+    // Disable V2 feature because LibAssistant V2 binary does not run on linux
+    // bot.
     if (GetParam()) {
-      feature_list_.InitAndEnableFeature(features::kEnableLibAssistantDlc);
+      feature_list_.InitWithFeatures(
+          /*enabled_features=*/{features::kEnableLibAssistantDlc},
+          /*disabled_features=*/{features::kEnableLibAssistantV2});
+    } else {
+      feature_list_.InitWithFeatures(
+          /*enabled_features=*/{},
+          /*disabled_features=*/{features::kEnableLibAssistantDlc,
+                                 features::kEnableLibAssistantV2});
     }
 
     // Do not log to file in test. Otherwise multiple tests may create/delete

diff --git a/chrome/browser/ui/ash/assistant/assistant_timers_browsertest.cc b/chrome/browser/ui/ash/assistant/assistant_timers_browsertest.cc
@@ -193,8 +193,17 @@ class AssistantTimersBrowserTest : public MixinBasedInProcessBrowserTest,
                                    public testing::WithParamInterface<bool> {
  public:
   AssistantTimersBrowserTest() {
+    // Disable V2 feature because LibAssistant V2 binary does not run on linux
+    // bot.
     if (GetParam()) {
-      feature_list_.InitAndEnableFeature(features::kEnableLibAssistantDlc);
+      feature_list_.InitWithFeatures(
+          /*enabled_features=*/{features::kEnableLibAssistantDlc},
+          /*disabled_features=*/{features::kEnableLibAssistantV2});
+    } else {
+      feature_list_.InitWithFeatures(
+          /*enabled_features=*/{},
+          /*disabled_features=*/{features::kEnableLibAssistantDlc,
+                                 features::kEnableLibAssistantV2});
     }
 
     // Do not log to file in test. Otherwise multiple tests may create/delete

diff --git a/chromeos/ash/services/assistant/public/cpp/features.cc b/chromeos/ash/services/assistant/public/cpp/features.cc
@@ -50,7 +50,7 @@ BASE_FEATURE(kDisableVoiceMatch,
 
 BASE_FEATURE(kEnableLibAssistantV2,
              "LibAssistantV2",
-             base::FEATURE_DISABLED_BY_DEFAULT);
+             base::FEATURE_ENABLED_BY_DEFAULT);
 
 BASE_FEATURE(kEnableLibAssistantDlc,
              "LibAssistantDlc",

diff --git a/chromeos/ash/services/libassistant/authentication_state_observer_unittest.cc b/chromeos/ash/services/libassistant/authentication_state_observer_unittest.cc
@@ -2,7 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
+#include "chromeos/ash/services/assistant/public/cpp/features.h"
 #include "chromeos/ash/services/libassistant/libassistant_service.h"
 #include "chromeos/ash/services/libassistant/public/mojom/authentication_state_observer.mojom.h"
 #include "chromeos/ash/services/libassistant/test_support/libassistant_service_tester.h"
@@ -76,6 +78,10 @@ class AuthenticationStateObserverTest : public ::testing::Test {
   ~AuthenticationStateObserverTest() override = default;
 
   void SetUp() override {
+    // TODO(b/269803444): Reenable tests for LibAssistantV2.
+    feature_list_.InitAndDisableFeature(
+        assistant::features::kEnableLibAssistantV2);
+
     service_tester_.service().AddAuthenticationStateObserver(
         observer_mock_.BindNewPipeAndPassRemote());
 
@@ -97,11 +103,12 @@ class AuthenticationStateObserverTest : public ::testing::Test {
 
  private:
   base::test::SingleThreadTaskEnvironment environment_;
+  base::test::ScopedFeatureList feature_list_;
   ::testing::StrictMock<AuthenticationStateObserverMock> observer_mock_;
   LibassistantServiceTester service_tester_;
 };
 
-TEST_F(AuthenticationStateObserverTest, ShouldReportAuthenticationErrors) {
+TEST_F(AuthenticationStateObserverTest, ShouldReportAuthenticationErrors_V1) {
   for (int code : GetAuthenticationErrorCodes()) {
     EXPECT_CALL(observer_mock(), OnAuthenticationError());
     assistant_manager_delegate().OnCommunicationError(code);
@@ -112,7 +119,8 @@ TEST_F(AuthenticationStateObserverTest, ShouldReportAuthenticationErrors) {
   }
 }
 
-TEST_F(AuthenticationStateObserverTest, ShouldIgnoreNonAuthenticationErrors) {
+TEST_F(AuthenticationStateObserverTest,
+       ShouldIgnoreNonAuthenticationErrors_V1) {
   std::vector<int> non_authentication_errors = GetNonAuthenticationErrorCodes();
 
   // check to ensure these are not authentication errors.

diff --git a/chromeos/ash/services/libassistant/conversation_observer_unittest.cc b/chromeos/ash/services/libassistant/conversation_observer_unittest.cc
@@ -131,7 +131,7 @@ class AssistantConversationObserverTest : public ::testing::Test {
 
     action_module_helper_ = std::make_unique<CrosActionModuleHelper>(
         static_cast<chromeos::assistant::action::CrosActionModule*>(
-            service_tester_.assistant_manager_internal().action_module()));
+            controller().action_module()));
   }
 
   assistant_client::ConversationStateListener& conversation_state_listener() {

diff --git a/chromeos/ash/services/libassistant/display_controller.h b/chromeos/ash/services/libassistant/display_controller.h
@@ -62,6 +62,10 @@ class DisplayController
       const std::vector<assistant::AndroidAppInfo>& apps_info,
       const chromeos::assistant::InteractionInfo& interaction) override;
 
+  DisplayConnection& GetDisplayConnectionForTesting() {
+    return *display_connection_.get();
+  }
+
  private:
   class EventObserver;
 

diff --git a/chromeos/ash/services/libassistant/grpc/BUILD.gn b/chromeos/ash/services/libassistant/grpc/BUILD.gn
@@ -149,6 +149,7 @@ source_set("unit_tests") {
     ":grpc_service",
     ":http_connection_client",
     "//base/test:test_support",
+    "//chromeos/ash/services/assistant/public/cpp",
     "//chromeos/assistant/internal",
     "//chromeos/assistant/internal:libassistant_shared_headers",
     "//chromeos/assistant/internal:test_support",

diff --git a/chromeos/ash/services/libassistant/grpc/assistant_client_v1_unittests.cc b/chromeos/ash/services/libassistant/grpc/assistant_client_v1_unittests.cc
@@ -6,6 +6,7 @@
 
 #include "base/functional/callback_helpers.h"
 #include "base/test/task_environment.h"
+#include "chromeos/ash/services/assistant/public/cpp/features.h"
 #include "chromeos/ash/services/libassistant/grpc/assistant_client_v1.h"
 #include "chromeos/ash/services/libassistant/grpc/services_status_observer.h"
 #include "chromeos/assistant/internal/test_support/fake_assistant_manager.h"
@@ -101,8 +102,12 @@ class AssistantClientV1Test : public testing::Test {
 
 TEST_F(AssistantClientV1Test, ShouldNotifyServicesStarted) {
   MockServicesStatusObserver services_status_observer;
-  EXPECT_CALL(services_status_observer,
-              OnServicesStatusChanged(ServicesStatus::ONLINE_BOOTING_UP));
+
+  // If LibAssistantV2 is enabled, this will no be called.
+  if (!assistant::features::IsLibAssistantV2Enabled()) {
+    EXPECT_CALL(services_status_observer,
+                OnServicesStatusChanged(ServicesStatus::ONLINE_BOOTING_UP));
+  }
 
   StartServices(&services_status_observer);
 }

diff --git a/chromeos/ash/services/libassistant/libassistant_loader_impl_unittest.cc b/chromeos/ash/services/libassistant/libassistant_loader_impl_unittest.cc
@@ -36,8 +36,12 @@ TEST_F(LibassistantLoaderImplTest, ShouldCreateInstance) {
 }
 
 TEST_F(LibassistantLoaderImplTest, ShouldRunCallbackWithoutDlcFeature) {
-  feature_list_.InitAndDisableFeature(
-      assistant::features::kEnableLibAssistantDlc);
+  // Enable LibAssistantV2 will also enable LibAssistantDlc. Therefore, in this
+  // test, we disable both.
+  feature_list_.InitWithFeatures(
+      /*enabled_features=*/{},
+      /*disabled_features=*/{assistant::features::kEnableLibAssistantDlc,
+                             assistant::features::kEnableLibAssistantV2});
 
   auto* loader = LibassistantLoaderImpl::GetInstance();
   EXPECT_TRUE(loader);

diff --git a/chromeos/ash/services/libassistant/libassistant_service.h b/chromeos/ash/services/libassistant/libassistant_service.h
@@ -70,6 +70,10 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) LibassistantService
 
   ServiceController& service_controller() { return service_controller_; }
 
+  DisplayController& GetDisplayControllerForTesting() {
+    return display_controller_;
+  }
+
  private:
   mojo::Receiver<mojom::LibassistantService> receiver_;
   mojo::Remote<mojom::PlatformDelegate> platform_delegate_;

diff --git a/chromeos/ash/services/libassistant/media_controller.cc b/chromeos/ash/services/libassistant/media_controller.cc
@@ -253,4 +253,14 @@ void MediaController::OnAssistantClientRunning(
   assistant_client->AddMediaActionFallbackEventObserver(events_observer_.get());
 }
 
+void MediaController::SendGrpcMessageForTesting(
+    const ::assistant::api::OnDeviceStateEventRequest& request) {
+  events_observer_->OnGrpcMessage(request);
+}
+
+void MediaController::SendGrpcMessageForTesting(
+    const ::assistant::api::OnMediaActionFallbackEventRequest& request) {
+  events_observer_->OnGrpcMessage(request);
+}
+
 }  // namespace ash::libassistant
diff --git a/chromeos/ash/services/libassistant/media_controller.h b/chromeos/ash/services/libassistant/media_controller.h
@@ -7,6 +7,7 @@
 
 #include "chromeos/ash/services/libassistant/grpc/assistant_client_observer.h"
 #include "chromeos/ash/services/libassistant/public/mojom/media_controller.mojom.h"
+#include "chromeos/assistant/internal/proto/shared/proto/v2/delegate/event_handler_interface.pb.h"
 #include "chromeos/assistant/internal/proto/shared/proto/v2/device_state_event.pb.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -32,6 +33,11 @@ class MediaController : public mojom::MediaController,
   // AssistantClientObserver implementation:
   void OnAssistantClientRunning(AssistantClient* assistant_client) override;
 
+  void SendGrpcMessageForTesting(
+      const ::assistant::api::OnDeviceStateEventRequest& request);
+  void SendGrpcMessageForTesting(
+      const ::assistant::api::OnMediaActionFallbackEventRequest& request);
+
  private:
   class GrpcEventsObserver;
 

diff --git a/chromeos/ash/services/libassistant/media_controller_unittest.cc b/chromeos/ash/services/libassistant/media_controller_unittest.cc
@@ -6,10 +6,13 @@
 
 #include "base/strings/stringprintf.h"
 #include "base/test/task_environment.h"
+#include "chromeos/ash/services/assistant/public/cpp/features.h"
+#include "chromeos/ash/services/libassistant/grpc/utils/media_status_utils.h"
 #include "chromeos/ash/services/libassistant/public/mojom/media_controller.mojom.h"
 #include "chromeos/ash/services/libassistant/test_support/fake_assistant_client.h"
 #include "chromeos/ash/services/libassistant/test_support/libassistant_service_tester.h"
 #include "chromeos/assistant/internal/libassistant/shared_headers.h"
+#include "chromeos/assistant/internal/proto/shared/proto/v2/delegate/event_handler_interface.pb.h"
 #include "chromeos/assistant/internal/test_support/fake_assistant_manager.h"
 #include "chromeos/assistant/internal/util_headers.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -137,11 +140,34 @@ class AssistantMediaControllerTest : public testing::Test {
     return *media_controller_;
   }
 
+  void SendPlaybackState(const assistant_client::MediaStatus& input) {
+    if (assistant::features::IsLibAssistantV2Enabled()) {
+      ::assistant::api::OnDeviceStateEventRequest request;
+      auto* status = request.mutable_event()
+                         ->mutable_on_state_changed()
+                         ->mutable_new_state()
+                         ->mutable_media_status();
+      ConvertMediaStatusToV2FromV1(input, status);
+      media_controller().SendGrpcMessageForTesting(request);
+    } else {
+      libassistant_media_manager().listener().OnPlaybackStateChange(input);
+    }
+  }
+
   void CallFallbackMediaHandler(const std::string& action,
                                 const std::string& action_proto) {
-    auto handler =
-        service_tester_.assistant_manager_internal().media_action_fallback();
-    handler(action, action_proto);
+    if (assistant::features::IsLibAssistantV2Enabled()) {
+      ::assistant::api::OnMediaActionFallbackEventRequest request;
+      auto* media_action =
+          request.mutable_event()->mutable_on_media_action_event();
+      media_action->set_action_name(action);
+      media_action->set_action_args(action_proto);
+      media_controller().SendGrpcMessageForTesting(request);
+    } else {
+      auto handler =
+          service_tester_.assistant_manager_internal().media_action_fallback();
+      handler(action, action_proto);
+    }
   }
 
   void FlushMojomPipes() {
@@ -246,7 +272,7 @@ TEST_F(AssistantMediaControllerTest, ShouldSendPlaybackStateChangeToDelegate) {
   input.metadata.album = "album";
   input.metadata.artist = "artist";
   input.metadata.title = "title";
-  libassistant_media_manager().listener().OnPlaybackStateChange(input);
+  SendPlaybackState(input);
   FlushMojomPipes();
 
   ASSERT_FALSE(actual.is_null());
@@ -273,7 +299,7 @@ TEST_F(AssistantMediaControllerTest, ShouldSendPlaybackStateToDelegate) {
 
     assistant_client::MediaStatus input;
     input.playback_state = pair.second;
-    libassistant_media_manager().listener().OnPlaybackStateChange(input);
+    SendPlaybackState(input);
     FlushMojomPipes();
 
     ASSERT_FALSE(actual.is_null());

diff --git a/chromeos/ash/services/libassistant/notification_delegate_unittest.cc b/chromeos/ash/services/libassistant/notification_delegate_unittest.cc
@@ -2,7 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
+#include "chromeos/ash/services/assistant/public/cpp/features.h"
 #include "chromeos/ash/services/libassistant/public/cpp/assistant_notification.h"
 #include "chromeos/ash/services/libassistant/public/mojom/notification_delegate.mojom-forward.h"
 #include "chromeos/ash/services/libassistant/test_support/libassistant_service_tester.h"
@@ -79,6 +81,10 @@ class NotificationDelegateTest : public ::testing::Test {
   ~NotificationDelegateTest() override = default;
 
   void SetUp() override {
+    // TODO(b/269803444): Reenable tests for LibAssistantV2.
+    feature_list_.InitAndDisableFeature(
+        assistant::features::kEnableLibAssistantV2);
+
     delegate_mock_.Bind(
         service_tester_.GetNotificationDelegatePendingReceiver());
     service_tester_.Start();
@@ -104,12 +110,13 @@ class NotificationDelegateTest : public ::testing::Test {
 
  private:
   base::test::SingleThreadTaskEnvironment environment_;
+  base::test::ScopedFeatureList feature_list_;
   ::testing::StrictMock<NotificationDelegateMock> delegate_mock_;
   LibassistantServiceTester service_tester_;
   std::unique_ptr<CrosActionModuleHelper> action_module_helper_;
 };
 
-TEST_F(NotificationDelegateTest, ShouldInvokeAddOrUpdateNotification) {
+TEST_F(NotificationDelegateTest, ShouldInvokeAddOrUpdateNotification_V1) {
   chromeos::assistant::action::Notification input_notification{
       /*title=*/"title",
       /*text=*/"text",
@@ -144,7 +151,7 @@ TEST_F(NotificationDelegateTest, ShouldInvokeAddOrUpdateNotification) {
   delegate_mock().FlushForTesting();
 }
 
-TEST_F(NotificationDelegateTest, ShouldInvokeRemoveAllNotifications) {
+TEST_F(NotificationDelegateTest, ShouldInvokeRemoveAllNotifications_V1) {
   EXPECT_CALL(delegate_mock(), RemoveAllNotifications(/*from_server=*/true));
 
   // Pass in empty |grouping_key| should trigger all notifications being
@@ -153,7 +160,8 @@ TEST_F(NotificationDelegateTest, ShouldInvokeRemoveAllNotifications) {
   delegate_mock().FlushForTesting();
 }
 
-TEST_F(NotificationDelegateTest, ShouldInvokeRemoveNotificationByGroupingKey) {
+TEST_F(NotificationDelegateTest,
+       ShouldInvokeRemoveNotificationByGroupingKey_V1) {
   const std::string grouping_id = "grouping-id";
   EXPECT_CALL(delegate_mock(), RemoveNotificationByGroupingKey(
                                    /*id=*/grouping_id, /*from_server=*/true));