From 692488a254c4cb12c4f0778d10e51716af07a49f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 27 Oct 2017 17:41:32 -0400 Subject: [PATCH] Un-disable WinMultiprocess-based tests in Chromium MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As of https://chromium.googlesource.com/chromium/src/+/00a0654929787f70b0cd81f30aa14e81c5e11b2f, crashpad_util_test is able to run in Chromium. It uses Chromium’s own base::TestLauncher rather than gtest’s RUN_ALL_TESTS() for proper integration with Swarming. Launching WinMultiprocess test children out of the same test executable via WinChildProcess is not compatible with Chromium’s parallel, shardy, Swarmy test launcher. When running these children, the standard gtest RUN_ALL_TESTS() launcher will now be used, even in Chromium. Two tests disabled in Chromium are now enabled: ExceptionHandlerServerTest.MultipleConnections and ScopedProcessSuspend.ScopedProcessSuspend. As part of this work, I discovered that disabled tests chosen to run via --gtest_also_run_disabled_tests did not actually work for WinMultiprocess-based tests, because gtest’s test launcher would refuse to run the child side of the test, believing it was disabled. This is fixed by always supplying --gtest_also_run_disabled_tests to WinChildProcess children, on the basis that if the parent is managing to run and it’s disabled, disabled tests must actually be enabled. Bug: crashpad:205 Change-Id: Ied22f16b9329ee13b6b07fd29de704f6fe2a058e Reviewed-on: https://chromium-review.googlesource.com/742462 Reviewed-by: Scott Graham --- test/gmock_main.cc | 47 ----------------------- test/gtest_main.cc | 47 ++++++++++++++++++----- test/test.gyp | 38 ++++++++++-------- test/win/win_child_process.cc | 19 +++++---- util/win/exception_handler_server_test.cc | 8 +--- util/win/scoped_process_suspend_test.cc | 8 +--- 6 files changed, 72 insertions(+), 95 deletions(-) delete mode 100644 test/gmock_main.cc diff --git a/test/gmock_main.cc b/test/gmock_main.cc deleted file mode 100644 index be043996..00000000 --- a/test/gmock_main.cc +++ /dev/null @@ -1,47 +0,0 @@ -// Copyright 2017 The Crashpad Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "gmock/gmock.h" -#include "gtest/gtest.h" -#include "test/gtest_disabled.h" -#include "test/main_arguments.h" - -#if defined(CRASHPAD_IN_CHROMIUM) -#include "base/bind.h" -#include "base/test/launcher/unit_test_launcher.h" -#include "base/test/test_suite.h" -#endif // CRASHPAD_IN_CHROMIUM - -int main(int argc, char* argv[]) { - crashpad::test::InitializeMainArguments(argc, argv); - testing::InitGoogleMock(&argc, argv); - testing::AddGlobalTestEnvironment( - crashpad::test::DisabledTestGtestEnvironment::Get()); - -#if defined(CRASHPAD_IN_CHROMIUM) - - // This supports --test-launcher-summary-output, which writes a JSON file - // containing test details needed by Swarming. - base::TestSuite test_suite(argc, argv); - return base::LaunchUnitTests( - argc, - argv, - base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); - -#else // CRASHPAD_IN_CHROMIUM - - return RUN_ALL_TESTS(); - -#endif // CRASHPAD_IN_CHROMIUM -} diff --git a/test/gtest_main.cc b/test/gtest_main.cc index 8842b6a3..1e9b6b11 100644 --- a/test/gtest_main.cc +++ b/test/gtest_main.cc @@ -12,10 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "build/build_config.h" #include "gtest/gtest.h" #include "test/gtest_disabled.h" #include "test/main_arguments.h" +#if defined(CRASHPAD_TEST_LAUNCHER_GMOCK) +#include "gmock/gmock.h" +#endif // CRASHPAD_TEST_LAUNCHER_GMOCK + +#if defined(OS_WIN) +#include "test/win/win_child_process.h" +#endif // OS_WIN + #if defined(CRASHPAD_IN_CHROMIUM) #include "base/bind.h" #include "base/test/launcher/unit_test_launcher.h" @@ -24,23 +33,41 @@ int main(int argc, char* argv[]) { crashpad::test::InitializeMainArguments(argc, argv); + +#if defined(CRASHPAD_TEST_LAUNCHER_GTEST) testing::InitGoogleTest(&argc, argv); +#elif defined(CRASHPAD_TEST_LAUNCHER_GMOCK) + testing::InitGoogleMock(&argc, argv); +#else // CRASHPAD_TEST_LAUNCHER_GTEST +#error #define CRASHPAD_TEST_LAUNCHER_GTEST or CRASHPAD_TEST_LAUNCHER_GMOCK +#endif // CRASHPAD_TEST_LAUNCHER_GTEST + testing::AddGlobalTestEnvironment( crashpad::test::DisabledTestGtestEnvironment::Get()); #if defined(CRASHPAD_IN_CHROMIUM) - // This supports --test-launcher-summary-output, which writes a JSON file - // containing test details needed by Swarming. - base::TestSuite test_suite(argc, argv); - return base::LaunchUnitTests( - argc, - argv, - base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); +#if defined(OS_WIN) + // Chromium’s test launcher interferes with WinMultiprocess-based tests. Allow + // their child processes to be launched by the standard gtest-based test + // runner. + const bool use_chromium_test_launcher = + !crashpad::test::WinChildProcess::IsChildProcess(); +#else // OS_WIN + constexpr bool use_chromium_test_launcher = true; +#endif // OS_WIN -#else // CRASHPAD_IN_CHROMIUM - - return RUN_ALL_TESTS(); + if (use_chromium_test_launcher) { + // This supports --test-launcher-summary-output, which writes a JSON file + // containing test details needed by Swarming. + base::TestSuite test_suite(argc, argv); + return base::LaunchUnitTests( + argc, + argv, + base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); + } #endif // CRASHPAD_IN_CHROMIUM + + return RUN_ALL_TESTS(); } diff --git a/test/test.gyp b/test/test.gyp index c9279bb7..931d9485 100644 --- a/test/test.gyp +++ b/test/test.gyp @@ -101,21 +101,6 @@ }], ], }, - { - 'target_name': 'crashpad_gtest_main', - 'type': 'static_library', - 'dependencies': [ - 'crashpad_test', - '../third_party/gtest/gtest.gyp:gtest', - '../third_party/mini_chromium/mini_chromium.gyp:base', - ], - 'include_dirs': [ - '..', - ], - 'sources': [ - 'gtest_main.cc', - ], - }, { 'target_name': 'crashpad_gmock_main', 'type': 'static_library', @@ -128,8 +113,29 @@ 'include_dirs': [ '..', ], + 'defines': [ + 'CRASHPAD_TEST_LAUNCHER_GMOCK=1', + ], 'sources': [ - 'gmock_main.cc', + 'gtest_main.cc', + ], + }, + { + 'target_name': 'crashpad_gtest_main', + 'type': 'static_library', + 'dependencies': [ + 'crashpad_test', + '../third_party/gtest/gtest.gyp:gtest', + '../third_party/mini_chromium/mini_chromium.gyp:base', + ], + 'include_dirs': [ + '..', + ], + 'defines': [ + 'CRASHPAD_TEST_LAUNCHER_GTEST=1', + ], + 'sources': [ + 'gtest_main.cc', ], }, ], diff --git a/test/win/win_child_process.cc b/test/win/win_child_process.cc index 74687ee1..14299e6b 100644 --- a/test/win/win_child_process.cc +++ b/test/win/win_child_process.cc @@ -182,17 +182,20 @@ std::unique_ptr WinChildProcess::Launch() { } // Build a command line for the child process that tells it only to run the - // current test, and to pass down the values of the pipe handles. + // current test, and to pass down the values of the pipe handles. Use + // --gtest_also_run_disabled_tests because the test may be DISABLED_, but if + // it managed to run in the parent, disabled tests must be running. const ::testing::TestInfo* const test_info = ::testing::UnitTest::GetInstance()->current_test_info(); std::wstring command_line = - TestPaths::Executable().value() + L" " + - base::UTF8ToUTF16(base::StringPrintf("--gtest_filter=%s.%s %s=0x%x|0x%x", - test_info->test_case_name(), - test_info->name(), - kIsMultiprocessChild, - HandleToInt(write_for_child.get()), - HandleToInt(read_for_child.get()))); + TestPaths::Executable().value() + + base::UTF8ToUTF16(base::StringPrintf( + " --gtest_filter=%s.%s %s=0x%x|0x%x --gtest_also_run_disabled_tests", + test_info->test_case_name(), + test_info->name(), + kIsMultiprocessChild, + HandleToInt(write_for_child.get()), + HandleToInt(read_for_child.get()))); // Command-line buffer cannot be constant, per CreateProcess signature. handles_for_parent->process = LaunchCommandLine(&command_line[0]); diff --git a/util/win/exception_handler_server_test.cc b/util/win/exception_handler_server_test.cc index 7ca329e5..6f880516 100644 --- a/util/win/exception_handler_server_test.cc +++ b/util/win/exception_handler_server_test.cc @@ -180,13 +180,7 @@ class TestClient final : public WinChildProcess { DISALLOW_COPY_AND_ASSIGN(TestClient); }; -// https://crashpad.chromium.org/bug/205 -#if defined(CRASHPAD_IN_CHROMIUM) -#define MAYBE_MultipleConnections DISABLED_MultipleConnections -#else // CRASHPAD_IN_CHROMIUM -#define MAYBE_MultipleConnections MultipleConnections -#endif // CRASHPAD_IN_CHROMIUM -TEST_F(ExceptionHandlerServerTest, MAYBE_MultipleConnections) { +TEST_F(ExceptionHandlerServerTest, MultipleConnections) { WinChildProcess::EntryPoint(); std::unique_ptr handles_1 = diff --git a/util/win/scoped_process_suspend_test.cc b/util/win/scoped_process_suspend_test.cc index fbb34b73..2d0f5a0b 100644 --- a/util/win/scoped_process_suspend_test.cc +++ b/util/win/scoped_process_suspend_test.cc @@ -89,13 +89,7 @@ class ScopedProcessSuspendTest final : public WinChildProcess { DISALLOW_COPY_AND_ASSIGN(ScopedProcessSuspendTest); }; -// https://crashpad.chromium.org/bug/205 -#if defined(CRASHPAD_IN_CHROMIUM) -#define MAYBE_ScopedProcessSuspend DISABLED_ScopedProcessSuspend -#else // CRASHPAD_IN_CHROMIUM -#define MAYBE_ScopedProcessSuspend ScopedProcessSuspend -#endif // CRASHPAD_IN_CHROMIUM -TEST(ScopedProcessSuspend, MAYBE_ScopedProcessSuspend) { +TEST(ScopedProcessSuspend, ScopedProcessSuspend) { WinChildProcess::EntryPoint(); std::unique_ptr handles = WinChildProcess::Launch();