[Test Automation] More elegant handling of platform incompatibilities

When writing tests using InteractionTestUtil and InteractiveTest[Api], you can now specify how you want to handle a known situation in which an action might not work on a particular platform, in a particular binary or environment, in a particular job, etc.

Examples include running a test with the Screenshot() verb outside of a pixel_tests job, or trying to activate an inactive surface on some flavors of Linux (it is, for example, not currently possible to raise a window programmatically in vanilla Wayland).

This provides us with an elegant way of handling known incompatibilities. Test-writers are given the option - if they know a particular step might fail due to such an incompatibility - to specify exactly how the test should handle the faulty step. Options include:
 - Fail (default)
 - Skip the test
 - Halt the test (and succeed if there are no other errors)
 - Ignore the step and continue

Specifying anything but the default requires a reason directly in the code itself, as this is an override of expected behavior and may limit test coverage depending on how it is used.

Tests are provided at every level to ensure correct behavior.

Bug: 1396074
Change-Id: I93a1bef95fcef6e917ceefc2efb546e4fccd5b2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4114944
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Kuan Huang <kuanhuang@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085188}

chrome/browser/ash/crosapi/test_controller_ash.cc

@@ -61,6 +61,7 @@
 #include "ui/events/event_source.h"
 #include "ui/events/types/event_type.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/views/controls/button/button.h"
 #include "ui/views/interaction/element_tracker_views.h"
 #include "ui/views/interaction/interaction_test_util_views.h"
 
@@ -137,20 +138,19 @@ void TestControllerAsh::ClickElement(const std::string& element_name,
   }
 
   // Pick the first view that matches the element name.
-  views::View* view = views[0];
+  views::Button* button = views::Button::AsButton(views[0]);
+  if (!button) {
+    std::move(callback).Run(/*success=*/false);
+    return;
+  }
 
   // We directly send mouse events to the view. It's also possible to use
   // EventGenerator to move the mouse and send a click. Unfortunately, that
   // approach has occasional flakiness. This is presumably due to another window
   // appearing on top of the dialog and taking the mouse events but has not been
   // explicitly diagnosed.
-  views::TrackedElementViews* tracked_element =
-      views::ElementTrackerViews::GetInstance()->GetElementForView(
-          view, /*assign_temporary_id=*/false);
-  views::test::InteractionTestUtilSimulatorViews simulator;
-  simulator.PressButton(tracked_element,
-                        ui::test::InteractionTestUtil::InputType::kMouse);
-
+  views::test::InteractionTestUtilSimulatorViews::PressButton(
+      button, ui::test::InteractionTestUtil::InputType::kMouse);
   std::move(callback).Run(/*success=*/true);
 }
 

chrome/browser/ui/views/user_education/help_bubble_factory_registry_interactive_uitest.cc

@@ -69,7 +69,8 @@ IN_PROC_BROWSER_TEST_F(HelpBubbleFactoryRegistryInteractiveUitest,
           kAppMenuButtonElementId, context);
   ASSERT_NE(nullptr, app_menu_button);
   InteractionTestUtilBrowser test_util;
-  test_util.PressButton(app_menu_button);
+  ASSERT_EQ(ui::test::ActionResult::kSucceeded,
+            test_util.PressButton(app_menu_button));
 
   // Verify that the history menu item is visible.
   ui::TrackedElement* const element =

chrome/browser/ui/views/user_education/tutorial_interactive_uitest.cc

@@ -106,7 +106,8 @@ IN_PROC_BROWSER_TEST_F(TutorialInteractiveUitest, SampleTutorial) {
       GetElement(kTabStripElementId), kCustomEventType1);
 
   InteractionTestUtilBrowser test_util;
-  test_util.PressButton(GetElement(kAppMenuButtonElementId));
+  EXPECT_EQ(ui::test::ActionResult::kSucceeded,
+            test_util.PressButton(GetElement(kAppMenuButtonElementId)));
 
   // Simulate click on close button.
   EXPECT_CALL_IN_SCOPE(

chrome/test/interaction/README.md

@@ -81,7 +81,8 @@ Verbs fall into a number of different categories:
     - `CheckView()` [Views]
     - `CheckViewProperty()` [Views]
     - `Screenshot` [Browser] - compares the target against Skia Gold in pixel
-      tests
+      tests. See [Handling Incompatibilities](#handling-incompatibilities) for
+      how to handle this in non-pixel tests.
 - **WaitFor** verbs ensure that the given UI event happens or condition becomes
   true before proceeding. Examples:
     - `WaitForShow()`
@@ -118,10 +119,13 @@ Verbs fall into a number of different categories:
     - `SelectTab()`
     - `SelectDropdownItem()`
     - `EnterText()`
-    - `ActivateSurface()`
     - `SendAccelerator()`
     - `Confirm()`
     - `DoDefaultAction()`
+    - `ActivateSurface()`
+      - ActivateSurface is not always reliable on Linux with the Wayland window
+        manager; see [Handling Incompatibilities](#handling-incompatibilities)
+        for how to correctly deal with this.
 - **Mouse** verbs simulate mouse input to the entire application, and are
   therefore only reliable in test fixtures that run as exclusive processes (e.g.
   interactive_browser_tests). Examples include:
@@ -160,9 +164,14 @@ Verbs fall into a number of different categories:
    - `ExecuteJsAt()` [Browser]
    - `CheckJsResult()` [Browser]
    - `CheckJsResultAt()` [Browser]
-- Utility verbs modify how the test sequence is executed. Currently there is
-  only `FlushEvents()`, which ensures that the next step happens on a fresh
-  message loop rather than being able to chain successive steps.
+- **Utility** verbs modify how the test sequence is executed.
+   - `FlushEvents()` - ensures that the next step happens on a fresh
+     message loop rather than being able to chain successive steps.
+   - `SetOnIncompatibleAction()` changes what the sequence will do when faced
+     with an action that cannot be executed on the current
+     build, environment, or platform. See
+     [Handling Incompatibilities](#handling-incompatibilities) for more
+     information and best practices.
 
 Example with mouse input:
 ```cpp
@@ -235,6 +244,45 @@ RunTestSequence(
     WaitForShow(kDownloadsMenuItemElementId))));
 ```
 
+### Handling Incompatibilities
+
+Sometimes a test won't run on a specific build bot or in a specific environment
+due to a known incompatibility (as opposed to something legitimately failing).
+Current known incompatibilities include:
+ - `ActivateSurface()` does not work on the `linux-wayland` buildbot unless the
+   surface is already active, due to vanilla Wayland not supporting programmatic
+   window activation.
+ - `Screenshot()` only works in specific pixel test jobs on the `win-rel`
+   buildbot.
+
+Normally, if you know that the test won't run on an entire platform (i.e. you
+can use `BUILDFLAG()` to differentiate) you should disable or skip the tests in
+the usual way. But if the distinction is finer-grained (as with the above verbs)
+The `SetOnIncompatibleAction()` verb and `OnIncompatibleAction` enumeration are
+provided.
+ - `OnIncompatibleAction::kFailTest` is the default option; if a step fails
+   because of a known incompatibility, the test will fail, and an error message
+   will be printed.
+ - `OnIncompatibleAction::kSkipTest` immediately skips the test as soon as an
+   incompatibility is detected. Use this option when you know the rest of the
+   test will fail and the test results are invalid. A warning will be printed.
+ - `OnIncompatibleAction::kHaltTest` immediately halts the sequence but does not
+   fail or skip the test. Use this option when all of the steps leading up to
+   the incompatible one are valid and you want to preserve any non-fatal errors
+   that may have occurred. A warning will still be printed.
+ - `OnIncompatibleAction::kIgnoreAndContinue` skips the problematic step, prints
+   a warning, and continues the test as if nothing happened. Use this option
+   when the step is incidental to the test, such as taking a screenshot in the
+   middle of a sequence.
+
+***Do not use `SetOnIncompatibleAction()` unless:***
+ 1. You know the test will fail due to a known incompatibility.
+ 2. The test cannot be disabled or skipped using a simple `BUILDFLAG()` check.
+
+Note that you *must* specify a non-empty `reason` when calling
+`SetOnIncompatibleAction()` with any argument except `kFailTest`. This string
+will be printed out as part of the warning that is produced if the step fails.
+
 ### WebContents Instrumentation
 
 A feature of `InteractiveBrowserTestApi` that it borrows from

chrome/test/interaction/interaction_test_util_browser.cc

@@ -8,6 +8,7 @@
 
 #include "base/strings/strcat.h"
 #include "build/build_config.h"
+#include "build/chromeos_buildflags.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -18,6 +19,7 @@
 #include "chrome/test/interaction/tracked_element_webcontents.h"
 #include "chrome/test/interaction/webcontents_interaction_test_util.h"
 #include "ui/base/interaction/element_tracker.h"
+#include "ui/base/interaction/interaction_test_util.h"
 #include "ui/base/test/ui_controls.h"
 #include "ui/events/event.h"
 #include "ui/events/types/event_type.h"
@@ -33,6 +35,7 @@
 
 #if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
 #define SUPPORTS_PIXEL_TESTS 1
+#include "base/command_line.h"
 #include "chrome/browser/ui/test/test_browser_ui.h"
 #else
 #define SUPPORTS_PIXEL_TESTS 0
@@ -86,29 +89,32 @@ class InteractionTestUtilSimulatorBrowser
   // Browser accelerators must be sent via key events to the window on Mac or
   // they don't work properly. Dialog accelerators still appear to work the same
   // as on other platforms.
-  bool SendAccelerator(ui::TrackedElement* element,
-                       const ui::Accelerator& accelerator) override {
+  ui::test::ActionResult SendAccelerator(ui::TrackedElement* element,
+                                         ui::Accelerator accelerator) override {
     Browser* const browser =
         InteractionTestUtilBrowser::GetBrowserFromContext(element->context());
     if (!browser)
-      return false;
-
-    CHECK(ui_controls::SendKeyPress(
-        browser->window()->GetNativeWindow(), accelerator.key_code(),
-        accelerator.IsCtrlDown(), accelerator.IsShiftDown(),
-        accelerator.IsAltDown(), accelerator.IsCmdDown()));
-
-    return true;
+      return ui::test::ActionResult::kNotAttempted;
+
+    if (!ui_controls::SendKeyPress(
+            browser->window()->GetNativeWindow(), accelerator.key_code(),
+            accelerator.IsCtrlDown(), accelerator.IsShiftDown(),
+            accelerator.IsAltDown(), accelerator.IsCmdDown())) {
+      LOG(ERROR) << "Failed to send accelerator"
+                 << accelerator.GetShortcutText() << " to " << *element;
+      return ui::test::ActionResult::kFailed;
+    }
+    return ui::test::ActionResult::kSucceeded;
   }
 #endif  // BUILDFLAG(IS_MAC)
 
-  bool SelectTab(ui::TrackedElement* tab_collection,
-                 size_t index,
-                 InputType input_type) override {
+  ui::test::ActionResult SelectTab(ui::TrackedElement* tab_collection,
+                                   size_t index,
+                                   InputType input_type) override {
     // This handler *explicitly* only handles Browser and TabStrip; it will
     // reject any other element or View type.
     if (!tab_collection->IsA<views::TrackedElementViews>())
-      return false;
+      return ui::test::ActionResult::kNotAttempted;
     auto* const view =
         tab_collection->AsA<views::TrackedElementViews>()->view();
     TabStrip* tab_strip = nullptr;
@@ -118,36 +124,43 @@ class InteractionTestUtilSimulatorBrowser
       tab_strip = views::AsViewClass<TabStrip>(view);
     }
     if (!tab_strip)
-      return false;
+      return ui::test::ActionResult::kNotAttempted;
 
     // Verify that the tab index is in range; at this point it's a fatal error
     // if it's out of bounds.
-    CHECK_LT(static_cast<int>(index), tab_strip->GetTabCount())
-        << "Tab strip tab index " << index << " is out of bounds.";
+    if (static_cast<int>(index) >= tab_strip->GetTabCount()) {
+      LOG(ERROR) << "Tabstrip index " << index
+                 << " is out of bounds, there are " << tab_strip->GetTabCount()
+                 << " tabs.";
+      return ui::test::ActionResult::kFailed;
+    }
 
     // Tabs can be selected using a default action; no special input logic is
     // needed.
     Tab* const tab = tab_strip->tab_at(index);
     views::test::InteractionTestUtilSimulatorViews::DoDefaultAction(tab,
                                                                     input_type);
-    CHECK_EQ(static_cast<int>(index), tab_strip->GetActiveIndex().value());
-    return true;
+    if (static_cast<int>(index) != tab_strip->GetActiveIndex()) {
+      LOG(ERROR) << "Failed to select tabstrip tab " << index;
+      return ui::test::ActionResult::kFailed;
+    }
+    return ui::test::ActionResult::kSucceeded;
   }
 
-  bool Confirm(ui::TrackedElement* element) override {
+  ui::test::ActionResult Confirm(ui::TrackedElement* element) override {
     // This handler *explicitly* only handles OmniboxView; it will reject any
     // other element or View type.
     if (!element->IsA<views::TrackedElementViews>())
-      return false;
+      return ui::test::ActionResult::kNotAttempted;
     auto* const view = element->AsA<views::TrackedElementViews>()->view();
     if (auto* const omnibox = views::AsViewClass<OmniboxViewViews>(view)) {
       ui::KeyEvent press(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::EF_NONE);
       omnibox->OnKeyEvent(&press);
       ui::KeyEvent release(ui::ET_KEY_RELEASED, ui::VKEY_RETURN, ui::EF_NONE);
       omnibox->OnKeyEvent(&release);
-      return true;
+      return ui::test::ActionResult::kSucceeded;
     }
-    return false;
+    return ui::test::ActionResult::kNotAttempted;
   }
 };
 
@@ -176,23 +189,37 @@ Browser* InteractionTestUtilBrowser::GetBrowserFromContext(
 }
 
 // static
-bool InteractionTestUtilBrowser::CompareScreenshot(
+ui::test::ActionResult InteractionTestUtilBrowser::CompareScreenshot(
     ui::TrackedElement* element,
     const std::string& screenshot_name,
     const std::string& baseline) {
-#if SUPPORTS_PIXEL_TESTS
   views::View* view = nullptr;
   if (auto* const view_el = element->AsA<views::TrackedElementViews>()) {
     view = view_el->view();
   } else if (auto* const page_el = element->AsA<TrackedElementWebContents>()) {
     view = page_el->owner()->GetWebView();
   }
+  if (!view) {
+    return ui::test::ActionResult::kNotAttempted;
+  }
 
-  CHECK(view);
+#if SUPPORTS_PIXEL_TESTS
+  // pixel_browser_tests and pixel_interactive_ui_tests specify this command
+  // line, which is checked by TestBrowserUi before attempting any screen
+  // capture; otherwise screenshotting is a silent no-op.
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          "browser-ui-tests-verify-pixels")) {
+    LOG(WARNING)
+        << "Cannot take screenshot: pixel test command line not set. This is "
+           "normal for non-pixel-test jobs such as vanilla browser_tests.";
+    return ui::test::ActionResult::kKnownIncompatible;
+  }
 
   PixelTestUi pixel_test_ui(view, screenshot_name, baseline);
-  return pixel_test_ui.VerifyUi();
+  return pixel_test_ui.VerifyUi() ? ui::test::ActionResult::kSucceeded
+                                  : ui::test::ActionResult::kFailed;
 #else  // !SUPPORTS_PIXEL_TESTS
-  return true;
+  LOG(WARNING) << "Current platform does not support pixel tests.";
+  return ui::test::ActionResult::kKnownIncompatible;
 #endif
 }

chrome/test/interaction/interaction_test_util_browser.h

@@ -25,8 +25,9 @@ class InteractionTestUtilBrowser : public ui::test::InteractionTestUtil {
   static Browser* GetBrowserFromContext(ui::ElementContext context);
 
   // Takes a screenshot based on the contents of `element` and compares with
-  // Skia Gold. On platforms where screenshots are unsupported or flaky, may
-  // trivially return true.
+  // Skia Gold. May return ActionResult::kKnownIncompatible on platforms and
+  // builds where screenshots are not supported or not reliable. This is not
+  // necessarily an error; the remainder of the test may still be valid.
   //
   // The name of the screenshot will be composed as follows:
   //   TestFixture_TestName[_screenshot_name]_baseline
@@ -57,9 +58,10 @@ class InteractionTestUtilBrowser : public ui::test::InteractionTestUtil {
   // activation, or occlusion could change the behavior of a test). So if you
   // need to both test complex interaction and take screenshots, prefer putting
   // your test in interactive_ui_tests.
-  static bool CompareScreenshot(ui::TrackedElement* element,
-                                const std::string& screenshot_name,
-                                const std::string& baseline);
+  static ui::test::ActionResult CompareScreenshot(
+      ui::TrackedElement* element,
+      const std::string& screenshot_name,
+      const std::string& baseline);
 };
 
 #endif  // CHROME_TEST_INTERACTION_INTERACTION_TEST_UTIL_BROWSER_H_