Reland "vm_tools: sommelier: Test handling of XCB_MAP_REQUEST."

This reverts commit 2fab0f00df6fe755a2149fc037d73d23bd64fd65. Reason for revert: Relanding with fixes. Replaced assert()s with gtest EXPECT macros so they don't get compiled out. Using EXPECT instead of ASSERT because the latter can only be used in void functions (see gtest docs). Original change's description: > Revert "vm_tools: sommelier: Test handling of XCB_MAP_REQUEST." > > This reverts commit 3b3e04abdeddc4fc3cb8e4a02528794169122922. > > Reason for revert: b/302429827 tatl,tael release builders failing to compile sommelier unit tests > > Original change's description: > > vm_tools: sommelier: Test handling of XCB_MAP_REQUEST. > > > > This requires a working model of the xcb_get_property*() functions, > > which is hard to create using mocks due to the required state tracking. > > Instead, we can use a simple fake. > > > > BUG=b:296327255 > > TEST=run unit tests: meson setup builddir; pushd builddir; ninja && ./sommelier_test; popd > > > > Change-Id: I33d53fd838211bc0a69e5befdf71b734177d917b > > Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4885941 > > Tested-by: Chloe Pelling <[email protected]> > > Commit-Queue: Chloe Pelling <[email protected]> > > Reviewed-by: Justin Huang <[email protected]> > > Auto-Submit: Chloe Pelling <[email protected]> > > BUG=b:296327255,b:302429827 > > Change-Id: If0211fd3f9dd30e716ef10d6d33e6fc49dbaf78c > Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4900441 > Commit-Queue: Chloe Pelling <[email protected]> > Commit-Queue: Sophia Lin <[email protected]> > Reviewed-by: Sophia Lin <[email protected]> > Tested-by: Chloe Pelling <[email protected]> > Auto-Submit: Chloe Pelling <[email protected]> BUG=b:296327255,b:302429827 TEST=run unit tests: meson setup builddir; pushd builddir; ninja && ./sommelier_test; popd Change-Id: I49b2c7ce1d1920e57b650d3330b21e04c5bbcc35 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4903443 Reviewed-by: Lucy Qu <[email protected]> Tested-by: Chloe Pelling <[email protected]> Commit-Queue: Chloe Pelling <[email protected]>
WhatAmISupposedToPutHere · Sep 29, 2023 · 8c47636 · 8c47636
1 parent a3b1478
commit 8c47636
Show file tree

Hide file tree

Showing 8 changed files with 522 additions and 11 deletions.
diff --git a/BUILD.gn b/BUILD.gn
@@ -174,9 +174,11 @@ if (use.test) {
       "sommelier-test.cc",
       "sommelier-transform-test.cc",
       "sommelier-window-test.cc",
+      "sommelier-x11event-test.cc",
       "sommelier-xdg-shell-test.cc",
       "testing/mock-wayland-channel.cc",
       "testing/sommelier-test-util.cc",
+      "xcb/fake-xcb-shim.cc",
     ]
     include_dirs = [ "testing" ]
 

diff --git a/meson.build b/meson.build
@@ -221,9 +221,11 @@ if get_option('with_tests')
       'sommelier-transform-test.cc',
       'sommelier-output-test.cc',
       'sommelier-window-test.cc',
+      'sommelier-x11event-test.cc',
       'sommelier-xdg-shell-test.cc',
       'testing/mock-wayland-channel.cc',
       'testing/sommelier-test-util.cc',
+      'xcb/fake-xcb-shim.cc',
     ] + wl_outs + shim_outs + gamepad_testing,
     link_with: libsommelier,
     dependencies: [

diff --git a/sommelier-output-test.cc b/sommelier-output-test.cc
@@ -167,7 +167,7 @@ TEST_F(X11Test, OutputsPositionedCorrectlyAfterRemovingLeftOutput) {
   EXPECT_EQ(output->virt_y, 0);
 
   // outputs has length 2.
-  EXPECT_EQ(ctx.host_outputs.size(), 2);
+  EXPECT_EQ(ctx.host_outputs.size(), 2u);
 }
 
 TEST_F(X11Test, OutputsPositionedCorrectlyAfterRemovingMiddleOutput) {
@@ -201,7 +201,7 @@ TEST_F(X11Test, OutputsPositionedCorrectlyAfterRemovingMiddleOutput) {
   EXPECT_EQ(output->virt_y, 0);
 
   // outputs has length 2.
-  EXPECT_EQ(ctx.host_outputs.size(), 2);
+  EXPECT_EQ(ctx.host_outputs.size(), 2u);
 }
 
 TEST_F(X11Test, OtherOutputUnchangedAfterRemovingRightOutput) {
@@ -222,7 +222,7 @@ TEST_F(X11Test, OtherOutputUnchangedAfterRemovingRightOutput) {
   EXPECT_EQ(output->virt_x, 0);
   EXPECT_EQ(output->virt_y, 0);
   // outputs has length 1.
-  EXPECT_EQ(ctx.host_outputs.size(), 1);
+  EXPECT_EQ(ctx.host_outputs.size(), 1u);
 }
 
 TEST_F(X11Test, RotatingOutputsShiftsNeighbouringOutputs) {

diff --git a/sommelier-x11event-test.cc b/sommelier-x11event-test.cc
@@ -0,0 +1,113 @@
+// Copyright 2023 The ChromiumOS Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <xcb/xproto.h>
+
+#include "testing/x11-test-base.h"
+#include "xcb/fake-xcb-shim.h"
+
+namespace vm_tools {
+namespace sommelier {
+
+using X11EventTest = X11TestBase;
+
+TEST_F(X11EventTest, MapRequestCreatesFrameWindow) {
+  sl_window* window = CreateWindowWithoutRole();
+  xcb_map_request_event_t event;
+  event.response_type = XCB_MAP_REQUEST;
+  event.window = window->id;
+  EXPECT_EQ(window->frame_id, XCB_WINDOW_NONE);
+
+  EXPECT_CALL(xcb, generate_id).WillOnce(testing::Return(456));
+  sl_handle_map_request(&ctx, &event);
+
+  EXPECT_EQ(window->frame_id, 456u);
+}
+
+TEST_F(X11EventTest, MapRequestIssuesMapWindow) {
+  sl_window* window = CreateWindowWithoutRole();
+  xcb_map_request_event_t event;
+  event.response_type = XCB_MAP_REQUEST;
+  event.window = window->id;
+
+  EXPECT_CALL(xcb, generate_id).WillOnce(testing::Return(456));
+  EXPECT_CALL(xcb, map_window(testing::_, window->id)).Times(1);
+  EXPECT_CALL(xcb, map_window(testing::_, 456u)).Times(1);
+
+  sl_handle_map_request(&ctx, &event);
+}
+
+TEST_F(X11EventTest, MapRequestGetsWmName) {
+  std::string windowName("Fred");
+  xcb.DelegateToFake();
+  sl_window* window = CreateWindowWithoutRole();
+  xcb.create_window(nullptr, 32, window->id, XCB_WINDOW_NONE, 0, 0, 800, 600, 0,
+                    XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0,
+                    nullptr);
+  xcb.change_property(nullptr, XCB_PROP_MODE_REPLACE, window->id,
+                      XCB_ATOM_WM_NAME, XCB_ATOM_STRING, 8, windowName.size(),
+                      windowName.c_str());
+  EXPECT_EQ(window->name, nullptr);
+
+  xcb_map_request_event_t event;
+  event.response_type = XCB_MAP_REQUEST;
+  event.window = window->id;
+  sl_handle_map_request(&ctx, &event);
+
+  EXPECT_EQ(window->name, windowName);
+}
+
+TEST_F(X11EventTest, ListensToWmNameChanges) {
+  std::string windowName("Fred");
+  xcb.DelegateToFake();
+  sl_window* window = CreateWindowWithoutRole();
+  xcb.create_window(nullptr, 32, window->id, XCB_WINDOW_NONE, 0, 0, 800, 600, 0,
+                    XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0,
+                    nullptr);
+  xcb.change_property(nullptr, XCB_PROP_MODE_REPLACE, window->id,
+                      XCB_ATOM_WM_NAME, XCB_ATOM_STRING, 8, windowName.size(),
+                      windowName.c_str());
+
+  xcb_property_notify_event_t event;
+  event.response_type = XCB_PROPERTY_NOTIFY;
+  event.window = window->id;
+  event.atom = XCB_ATOM_WM_NAME;
+  event.state = XCB_PROPERTY_NEW_VALUE;
+  sl_handle_property_notify(&ctx, &event);
+
+  EXPECT_EQ(window->name, windowName);
+}
+
+TEST_F(X11EventTest, NetWmNameOverridesWmname) {
+  std::string boringWindowName("Fred");
+  std::string fancyWindowName("I ♥️ Unicode 🦄🌈");
+  xcb.DelegateToFake();
+  sl_window* window = CreateWindowWithoutRole();
+  xcb.create_window(nullptr, 32, window->id, XCB_WINDOW_NONE, 0, 0, 800, 600, 0,
+                    XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0,
+                    nullptr);
+  xcb.change_property(nullptr, XCB_PROP_MODE_REPLACE, window->id,
+                      XCB_ATOM_WM_NAME, XCB_ATOM_STRING, 8,
+                      boringWindowName.size(), boringWindowName.c_str());
+  xcb.change_property(nullptr, XCB_PROP_MODE_REPLACE, window->id,
+                      ctx.atoms[ATOM_NET_WM_NAME].value, XCB_ATOM_STRING, 8,
+                      fancyWindowName.size(), fancyWindowName.c_str());
+
+  xcb_property_notify_event_t event;
+  event.response_type = XCB_PROPERTY_NOTIFY;
+  event.window = window->id;
+  event.atom = XCB_ATOM_WM_NAME;
+  event.state = XCB_PROPERTY_NEW_VALUE;
+  sl_handle_property_notify(&ctx, &event);
+
+  event.atom = ctx.atoms[ATOM_NET_WM_NAME].value;
+  sl_handle_property_notify(&ctx, &event);
+
+  EXPECT_EQ(window->name, fancyWindowName);
+}
+
+}  // namespace sommelier
+}  // namespace vm_tools
diff --git a/testing/x11-test-base.h b/testing/x11-test-base.h
@@ -20,6 +20,10 @@ class X11TestBase : public WaylandTestBase {
     WaylandTestBase::InitContext();
     ctx.xwayland = 1;
 
+    // Always delegate ID generation to the fake XCB shim, even for test cases
+    // that never use the fake for anything else. This prevents ID collisions.
+    xcb.DelegateIdGenerationToFake();
+
     // Create a fake screen with somewhat plausible values.
     // Some of these are not realistic because they refer to things not present
     // in the mocked X environment (such as specifying a root window with ID 0).
@@ -56,13 +60,8 @@ class X11TestBase : public WaylandTestBase {
 
   ~X11TestBase() override { set_xcb_shim(nullptr); }
 
-  uint32_t GenerateId() {
-    static uint32_t id = 0;
-    return ++id;
-  }
-
   virtual sl_window* CreateWindowWithoutRole() {
-    xcb_window_t window_id = GenerateId();
+    xcb_window_t window_id = xcb.generate_id(ctx.connection);
     sl_create_window(&ctx, window_id, 0, 0, 800, 600, 0);
     sl_window* window = sl_lookup_window(&ctx, window_id);
     EXPECT_NE(window, nullptr);
@@ -73,7 +72,7 @@ class X11TestBase : public WaylandTestBase {
     sl_window* window = CreateWindowWithoutRole();
 
     // Pretend we created a frame window too
-    window->frame_id = GenerateId();
+    window->frame_id = xcb.generate_id(ctx.connection);
 
     window->host_surface_id = SurfaceId(xwayland->CreateSurface());
     sl_window_update(window);

diff --git a/xcb/fake-xcb-shim.cc b/xcb/fake-xcb-shim.cc
@@ -0,0 +1,191 @@
+// Copyright 2023 The ChromiumOS Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <cstring>
+#include <gtest/gtest.h>
+#include <iostream>
+#include <utility>
+
+#include "fake-xcb-shim.h"  // NOLINT(build/include_directory)
+
+xcb_connection_t* FakeXcbShim::connect(const char* displayname, int* screenp) {
+  return nullptr;
+}
+
+uint32_t FakeXcbShim::generate_id(xcb_connection_t* c) {
+  return next_id_++;
+}
+
+xcb_void_cookie_t FakeXcbShim::create_window(xcb_connection_t* c,
+                                             uint8_t depth,
+                                             xcb_window_t wid,
+                                             xcb_window_t parent,
+                                             int16_t x,
+                                             int16_t y,
+                                             uint16_t width,
+                                             uint16_t height,
+                                             uint16_t border_width,
+                                             uint16_t _class,
+                                             xcb_visualid_t visual,
+                                             uint32_t value_mask,
+                                             const void* value_list) {
+  std::pair<std::unordered_map<xcb_window_t, FakeWindow>::iterator, bool>
+      result = windows_.try_emplace(wid, FakeWindow{
+                                             .depth = depth,
+                                             .wid = wid,
+                                             .parent = parent,
+                                             .x = x,
+                                             .y = y,
+                                             .width = width,
+                                             .height = height,
+                                             .border_width = border_width,
+                                             ._class = _class,
+                                             .visual = visual,
+                                         });
+  // Window should not already exist.
+  EXPECT_TRUE(result.second);
+  return {};
+}
+
+xcb_void_cookie_t FakeXcbShim::reparent_window(xcb_connection_t* c,
+                                               xcb_window_t window,
+                                               xcb_window_t parent,
+                                               int16_t x,
+                                               int16_t y) {
+  windows_.at(window).parent = parent;
+  return {};
+}
+
+xcb_void_cookie_t FakeXcbShim::map_window(xcb_connection_t* c,
+                                          xcb_window_t window) {
+  windows_.at(window).mapped = true;
+  return {};
+}
+
+xcb_void_cookie_t FakeXcbShim::configure_window(xcb_connection_t* c,
+                                                xcb_window_t window,
+                                                uint16_t value_mask,
+                                                const void* value_list) {
+  ADD_FAILURE() << "unimplemented";
+  return {};
+}
+
+xcb_void_cookie_t FakeXcbShim::change_property(xcb_connection_t* c,
+                                               uint8_t mode,
+                                               xcb_window_t window,
+                                               xcb_atom_t property,
+                                               xcb_atom_t type,
+                                               uint8_t format,
+                                               uint32_t data_len,
+                                               const void* data) {
+  // prepend/append not implemented
+  EXPECT_EQ(mode, XCB_PROP_MODE_REPLACE);
+
+  // The real API returns BadWindow errors if an invalid window ID is passed,
+  // but throwing an exception is sufficient for the purposes of our tests.
+  uint32_t bytes_per_element = format / 8;
+  windows_.at(window).properties_[property] = FakeProperty{
+      .type = type,
+      .format = format,
+      .data = std::vector<unsigned char>(
+          (unsigned char*)data,
+          (unsigned char*)data + data_len * bytes_per_element),
+  };
+  return {};
+}
+
+xcb_void_cookie_t FakeXcbShim::send_event(xcb_connection_t* c,
+                                          uint8_t propagate,
+                                          xcb_window_t destination,
+                                          uint32_t event_mask,
+                                          const char* event) {
+  ADD_FAILURE() << "unimplemented";
+  return {};
+}
+
+xcb_void_cookie_t FakeXcbShim::change_window_attributes(
+    xcb_connection_t* c,
+    xcb_window_t window,
+    uint32_t value_mask,
+    const void* value_list) {
+  ADD_FAILURE() << "unimplemented";
+  return {};
+}
+
+xcb_get_geometry_cookie_t FakeXcbShim::get_geometry(xcb_connection_t* c,
+                                                    xcb_drawable_t drawable) {
+  ADD_FAILURE() << "unimplemented";
+  return {};
+}
+
+xcb_get_geometry_reply_t* FakeXcbShim::get_geometry_reply(
+    xcb_connection_t* c,
+    xcb_get_geometry_cookie_t cookie,
+    xcb_generic_error_t** e) {
+  ADD_FAILURE() << "unimplemented";
+  return {};
+}
+
+xcb_get_property_cookie_t FakeXcbShim::get_property(xcb_connection_t* c,
+                                                    uint8_t _delete,
+                                                    xcb_window_t window,
+                                                    xcb_atom_t property,
+                                                    xcb_atom_t type,
+                                                    uint32_t long_offset,
+                                                    uint32_t long_length) {
+  xcb_get_property_cookie_t cookie;
+  cookie.sequence = next_cookie_++;
+
+  PropertyRequestData request;
+  request.window = window;
+  request.property = property;
+
+  property_requests_[cookie.sequence] = request;
+  return cookie;
+}
+
+xcb_get_property_reply_t* FakeXcbShim::get_property_reply(
+    xcb_connection_t* c,
+    xcb_get_property_cookie_t cookie,
+    xcb_generic_error_t** e) {
+  auto w = windows_.at(property_requests_[cookie.sequence].window);
+  xcb_atom_t property = property_requests_[cookie.sequence].property;
+  auto it = w.properties_.find(property);
+
+  // The caller is expected to call free() on the return value, so we must use
+  // malloc() here.
+  xcb_get_property_reply_t* reply = static_cast<xcb_get_property_reply_t*>(
+      malloc(sizeof(xcb_get_property_reply_t)));
+  EXPECT_TRUE(reply);
+  if (it == w.properties_.end()) {
+    reply->format = 0;
+    reply->sequence = 0;
+    reply->type = 0;
+    reply->length = 0;
+  } else {
+    reply->format = it->second.format;
+    reply->sequence = cookie.sequence;
+    reply->type = it->second.type;
+    reply->length = it->second.data.size();
+  }
+  return reply;
+}
+
+void* FakeXcbShim::get_property_value(const xcb_get_property_reply_t* r) {
+  if (!r->sequence) {
+    // Property was not found. This isn't an error case, just return null.
+    return nullptr;
+  }
+  const FakeProperty& property =
+      windows_.at(property_requests_[r->sequence].window)
+          .properties_.at(property_requests_[r->sequence].property);
+  void* buffer = malloc(property.data.size());
+  EXPECT_TRUE(buffer);
+  memcpy(buffer, property.data.data(), property.data.size());
+  return buffer;
+}
+
+int FakeXcbShim::get_property_value_length(const xcb_get_property_reply_t* r) {
+  return r->length;
+}