[Win] Create ModuleWatcher.

chrome/common/conflicts/module_watcher_win_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/common/conflicts/module_watcher_win.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class ModuleWatcherTest : public testing::Test {
+ public:
+  ModuleWatcherTest()
+      : module_(nullptr),
+        module_event_count_(0),
+        module_already_loaded_event_count_(0),
+        module_loaded_event_count_(0),
+        module_unloaded_event_count_(0) {}
+
+  void OnModuleEvent(const mojom::ModuleEvent& event) {
+    module_event_count_++;

[grt (UTC plus 2), 2016/11/10 08:52:13]

prefer preincrement always

[chrisha, 2016/11/14 16:06:45]

Done.

+    switch (event.event_type) {
+      case mojom::ModuleEventType::MODULE_ALREADY_LOADED:
+        module_already_loaded_event_count_++;
+        break;
+      case mojom::ModuleEventType::MODULE_LOADED:
+        module_loaded_event_count_++;
+        break;
+      case mojom::ModuleEventType::MODULE_UNLOADED:
+        module_unloaded_event_count_++;
+        break;
+    }
+  }
+
+  void TearDown() override { UnloadModule(); }
+
+  void LoadModule() {
+    if (module_)
+      return;
+    // This module should not be a static dependency of the unit-test
+    // executable, but should be a build-system dependency or a module that is
+    // present on any Windows machine.
+    static const wchar_t kModuleName[] = L"chrome.dll";

[grt (UTC plus 2), 2016/11/10 08:52:13]

nit: static constexpr

[grt (UTC plus 2), 2016/11/10 08:52:13]

is chrome.dll already a datadep of the unit_tests target? if it isn't, is there
something else you can use so that building unit_tests doesn't require waiting
for chrome.dll to link? chrome_elf is at least a whole lot smaller than
chrome.dll.

[chrisha, 2016/11/14 16:06:45]

Yeah, it's already a dependency of the unit_tests target. I could definitely use
chrome_elf instead.

I initially considered building my own test library just for this purpose, but
that seems like a bit of overkill.

[chrisha, 2016/11/14 16:06:45]

Done.

+    // The module should not already be loaded.
+    ASSERT_FALSE(::GetModuleHandle(kModuleName));
+    // It should load successfully.
+    module_ = ::LoadLibrary(kModuleName);
+    ASSERT_TRUE(module_);
+  }
+
+  void UnloadModule() {
+    if (!module_)
+      return;
+    ::FreeLibrary(module_);
+    module_ = nullptr;
+  }
+
+  std::unique_ptr<ModuleWatcher> Create() {
+    return ModuleWatcher::Create(
+        base::Bind(&ModuleWatcherTest::OnModuleEvent, base::Unretained(this)));
+  }
+
+  // Holds a handle to a loaded module.
+  HMODULE module_;
+
+  // Total number of module events seen.
+  size_t module_event_count_;
+  // Total number of MODULE_ALREADY_LOADED events seen.
+  size_t module_already_loaded_event_count_;
+  // Total number of MODULE_LOADED events seen.
+  size_t module_loaded_event_count_;
+  // Total number of MODULE_UNLOADED events seen.
+  size_t module_unloaded_event_count_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ModuleWatcherTest);
+};
+
+TEST_F(ModuleWatcherTest, SingleModuleWatcherOnly) {
+  std::unique_ptr<ModuleWatcher> mw1(Create());
+  EXPECT_TRUE(mw1.get());
+
+  std::unique_ptr<ModuleWatcher> mw2(Create());
+  EXPECT_FALSE(mw2.get());
+}
+
+TEST_F(ModuleWatcherTest, ModuleEvents) {
+  // Create the module watcher. This should immediately enumerate all already
+  // loaded modules.
+  std::unique_ptr<ModuleWatcher> mw(Create());
+  EXPECT_LT(0u, module_event_count_);

[grt (UTC plus 2), 2016/11/10 08:52:13]

the counts are unsigned, so just check for equality with zero.

[chrisha, 2016/11/14 16:06:45]

You're suggested that _NE is more readable that _LT? _LT more precisely states
what I mean, and doesn't require the additional contextual knowledge that the
count is an unsigned quantity.

[grt (UTC plus 2), 2016/11/14 20:30:31]

okay. now that i think about it, the counts should all be of type "int" as per
https://google.github.io/styleguide/cppguide.html#Integer_Types, making _LT even
more better.

+  EXPECT_LT(0u, module_already_loaded_event_count_);
+  EXPECT_EQ(0u, module_loaded_event_count_);
+  EXPECT_EQ(0u, module_unloaded_event_count_);
+
+  // Dynamically load a module and ensure it a notification is received for it.
+  LoadModule();
+  EXPECT_EQ(1u, module_loaded_event_count_);

[grt (UTC plus 2), 2016/11/10 08:52:13]

does this assume that the loaded module doesn't also bring in dependencies? if
so, maybe it's better to make your own test binary that you control.

[chrisha, 2016/11/14 16:06:45]

I just want to ensure that at least one module gets loaded in response to my
LoadLibrary, so it's fine if there are other dependencies. I'll update this
statement to reflect that.

[grt (UTC plus 2), 2016/11/14 20:30:31]

great. that addresses my concern -- that in some configurations there are
multiple loads.

+
+  // Unload the module and ensure another notification is received.
+  UnloadModule();
+  EXPECT_EQ(1u, module_unloaded_event_count_);
+
+  // Dynamically load a module and ensure it a notification is received for it.
+  LoadModule();
+  EXPECT_EQ(2u, module_loaded_event_count_);
+
+  // Destroy the module watcher.
+  mw.reset();
+
+  // Unload the module and ensure no notification is received this time.
+  UnloadModule();
+  EXPECT_EQ(1u, module_unloaded_event_count_);
+}