From 32a9d410ca0b22a17f5eade86057e6c4cdfa7f81 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 9 Mar 2015 15:13:13 -0400 Subject: [PATCH] Locate test data more robustly. Test code that requires test data should call Paths::TestDataRoot() to obtain the test data root. This will use the CRASHPAD_TEST_DATA_ROOT environment variable if set. Otherwise, it will look for test data at known locations relative to the executable path. If the test data is not found in any of these locations, it falls back to using the working directory, the same as the current behavior. BUG=crashpad:4 TEST=crashpad_util_test Paths.TestDataRoot and others R=rsesek@chromium.org, scottmg@chromium.org Review URL: https://codereview.chromium.org/992503002 --- build/run_tests.py | 5 +- util/net/http_body_test.cc | 14 ++- util/net/http_multipart_builder_test.cc | 35 +++--- util/net/http_transport_test.cc | 11 +- util/test/executable_path.h | 29 ----- util/test/multiprocess_exec_test.cc | 4 +- util/test/paths.cc | 101 ++++++++++++++++++ util/test/paths.h | 49 +++++++++ .../{executable_path_mac.cc => paths_mac.cc} | 5 +- ...{executable_path_test.cc => paths_test.cc} | 16 ++- util/test/paths_test_data_root.txt | 16 +++ .../{executable_path_win.cc => paths_win.cc} | 5 +- util/util.gyp | 9 +- util/win/process_info_test.cc | 4 +- 14 files changed, 225 insertions(+), 78 deletions(-) delete mode 100644 util/test/executable_path.h create mode 100644 util/test/paths.cc create mode 100644 util/test/paths.h rename util/test/{executable_path_mac.cc => paths_mac.cc} (93%) rename util/test/{executable_path_test.cc => paths_test.cc} (69%) create mode 100644 util/test/paths_test_data_root.txt rename util/test/{executable_path_win.cc => paths_win.cc} (92%) diff --git a/build/run_tests.py b/build/run_tests.py index bec22cfc..0f97ac41 100755 --- a/build/run_tests.py +++ b/build/run_tests.py @@ -27,13 +27,10 @@ def main(args): print >>sys.stderr, 'usage: run_tests.py {Debug|Release}' return 1; - # Until https://code.google.com/p/crashpad/issues/detail?id=4 is fixed, tests - # need to be run from a specific working directory. crashpad_dir = \ os.path.join(os.path.dirname(os.path.abspath(__file__)), os.pardir) - os.chdir(crashpad_dir) - binary_dir = os.path.join('out', args[0]) + binary_dir = os.path.join(crashpad_dir, 'out', args[0]) tests = [ 'crashpad_client_test', 'crashpad_minidump_test', diff --git a/util/net/http_body_test.cc b/util/net/http_body_test.cc index 8dbd5a10..8e58c205 100644 --- a/util/net/http_body_test.cc +++ b/util/net/http_body_test.cc @@ -16,6 +16,7 @@ #include "gtest/gtest.h" #include "util/net/http_body_test_util.h" +#include "util/test/paths.h" namespace crashpad { namespace test { @@ -93,9 +94,7 @@ TEST(StringHTTPBodyStream, MultipleReads) { } TEST(FileHTTPBodyStream, ReadASCIIFile) { - // TODO(rsesek): Use a more robust mechanism to locate testdata - // . - base::FilePath path = base::FilePath( + base::FilePath path = Paths::TestDataRoot().Append( FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt")); FileHTTPBodyStream stream(path); std::string contents = ReadStreamToString(&stream, 32); @@ -113,9 +112,7 @@ TEST(FileHTTPBodyStream, ReadASCIIFile) { TEST(FileHTTPBodyStream, ReadBinaryFile) { // HEX contents of file: |FEEDFACE A11A15|. - // TODO(rsesek): Use a more robust mechanism to locate testdata - // . - base::FilePath path = base::FilePath( + base::FilePath path = Paths::TestDataRoot().Append( FILE_PATH_LITERAL("util/net/testdata/binary_http_body.dat")); // This buffer size was chosen so that reading the file takes multiple reads. uint8_t buf[4]; @@ -199,8 +196,9 @@ TEST_P(CompositeHTTPBodyStreamBufferSize, StringsAndFile) { std::vector parts; parts.push_back(new StringHTTPBodyStream(string1)); - parts.push_back(new FileHTTPBodyStream(base::FilePath( - FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt")))); + base::FilePath path = Paths::TestDataRoot().Append( + FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt")); + parts.push_back(new FileHTTPBodyStream(path)); parts.push_back(new StringHTTPBodyStream(string2)); CompositeHTTPBodyStream stream(parts); diff --git a/util/net/http_multipart_builder_test.cc b/util/net/http_multipart_builder_test.cc index cb3fbaa7..e185e4ac 100644 --- a/util/net/http_multipart_builder_test.cc +++ b/util/net/http_multipart_builder_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" #include "util/net/http_body.h" #include "util/net/http_body_test_util.h" +#include "util/test/paths.h" namespace crashpad { namespace test { @@ -95,22 +96,19 @@ TEST(HTTPMultipartBuilder, ThreeStringFields) { TEST(HTTPMultipartBuilder, ThreeFileAttachments) { HTTPMultipartBuilder builder; - // TODO(rsesek): Use a more robust mechanism to locate testdata - // . + base::FilePath ascii_http_body_path = Paths::TestDataRoot().Append( + FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt")); builder.SetFileAttachment("first", "minidump.dmp", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/ascii_http_body.txt")), + ascii_http_body_path, ""); builder.SetFileAttachment("second", "minidump.dmp", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/ascii_http_body.txt")), + ascii_http_body_path, "text/plain"); builder.SetFileAttachment("\"third 50% silly\"", "test%foo.txt", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/ascii_http_body.txt")), + ascii_http_body_path, "text/plain"); const char kFileContents[] = "This is a test.\n"; @@ -182,22 +180,22 @@ TEST(HTTPMultipartBuilder, OverwriteFileAttachment) { HTTPMultipartBuilder builder; const char kValue[] = "1 2 3 test"; builder.SetFormData("a key", kValue); - // TODO(rsesek): Use a more robust mechanism to locate testdata - // . + base::FilePath testdata_path = + Paths::TestDataRoot().Append(FILE_PATH_LITERAL("util/net/testdata")); builder.SetFileAttachment("minidump", "minidump.dmp", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/binary_http_body.dat")), + testdata_path.Append(FILE_PATH_LITERAL( + "binary_http_body.dat")), ""); builder.SetFileAttachment("minidump2", "minidump.dmp", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/binary_http_body.dat")), + testdata_path.Append(FILE_PATH_LITERAL( + "binary_http_body.dat")), ""); builder.SetFileAttachment("minidump", "minidump.dmp", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/ascii_http_body.txt")), + testdata_path.Append(FILE_PATH_LITERAL( + "ascii_http_body.txt")), "text/plain"); scoped_ptr body(builder.GetBodyStream()); ASSERT_TRUE(body.get()); @@ -239,10 +237,11 @@ TEST(HTTPMultipartBuilder, SharedFormDataAndAttachmentKeyNamespace) { HTTPMultipartBuilder builder; const char kValue1[] = "11111"; builder.SetFormData("one", kValue1); + base::FilePath ascii_http_body_path = Paths::TestDataRoot().Append( + FILE_PATH_LITERAL("util/net/testdata/ascii_http_body.txt")); builder.SetFileAttachment("minidump", "minidump.dmp", - base::FilePath(FILE_PATH_LITERAL( - "util/net/testdata/ascii_http_body.txt")), + ascii_http_body_path, ""); const char kValue2[] = "this is not a file"; builder.SetFormData("minidump", kValue2); diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index 633ed341..ddda100e 100644 --- a/util/net/http_transport_test.cc +++ b/util/net/http_transport_test.cc @@ -20,11 +20,13 @@ #include +#include "base/files/file_path.h" #include "base/format_macros.h" #include "base/logging.h" #include "base/memory/scoped_ptr.h" #include "base/rand_util.h" #include "base/strings/stringprintf.h" +#include "base/strings/utf_string_conversions.h" #include "build/build_config.h" #include "gtest/gtest.h" #include "util/file/file_io.h" @@ -32,6 +34,7 @@ #include "util/net/http_headers.h" #include "util/net/http_multipart_builder.h" #include "util/test/multiprocess_exec.h" +#include "util/test/paths.h" namespace crashpad { namespace test { @@ -51,17 +54,17 @@ class HTTPTransportTestFixture : public MultiprocessExec { body_stream_(body_stream.Pass()), response_code_(http_response_code), request_validator_(request_validator) { - // TODO(rsesek): Use a more robust mechanism to locate testdata - // . + base::FilePath server_path = Paths::TestDataRoot().Append( + FILE_PATH_LITERAL("util/net/http_transport_test_server.py")); #if defined(OS_POSIX) - SetChildCommand("util/net/http_transport_test_server.py", nullptr); + SetChildCommand(server_path.value(), nullptr); #elif defined(OS_WIN) // Explicitly invoke a shell and python so that python can be found in the // path, and run the test script. std::vector args; args.push_back("/c"); args.push_back("python"); - args.push_back("util/net/http_transport_test_server.py"); + args.push_back(base::UTF16ToUTF8(server_path.value())); SetChildCommand(getenv("COMSPEC"), &args); #endif // OS_POSIX } diff --git a/util/test/executable_path.h b/util/test/executable_path.h deleted file mode 100644 index 4db0bea5..00000000 --- a/util/test/executable_path.h +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2014 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. - -#ifndef CRASHPAD_UTIL_TEST_EXECUTABLE_PATH_H_ -#define CRASHPAD_UTIL_TEST_EXECUTABLE_PATH_H_ - -#include "base/files/file_path.h" - -namespace crashpad { -namespace test { - -//! \brief Returns the pathname of the currently-running test executable. -base::FilePath ExecutablePath(); - -} // namespace test -} // namespace crashpad - -#endif // CRASHPAD_UTIL_TEST_EXECUTABLE_PATH_H_ diff --git a/util/test/multiprocess_exec_test.cc b/util/test/multiprocess_exec_test.cc index d151d2b1..8a5a6a95 100644 --- a/util/test/multiprocess_exec_test.cc +++ b/util/test/multiprocess_exec_test.cc @@ -19,7 +19,7 @@ #include "build/build_config.h" #include "gtest/gtest.h" #include "util/file/file_io.h" -#include "util/test/executable_path.h" +#include "util/test/paths.h" namespace crashpad { namespace test { @@ -48,7 +48,7 @@ class TestMultiprocessExec final : public MultiprocessExec { TEST(MultiprocessExec, MultiprocessExec) { TestMultiprocessExec multiprocess_exec; - base::FilePath test_executable = ExecutablePath(); + base::FilePath test_executable = Paths::Executable(); #if defined(OS_POSIX) std::string child_test_executable = test_executable.value(); #elif defined(OS_WIN) diff --git a/util/test/paths.cc b/util/test/paths.cc new file mode 100644 index 00000000..5634b62d --- /dev/null +++ b/util/test/paths.cc @@ -0,0 +1,101 @@ +// Copyright 2015 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 "util/test/paths.h" + +#include +#include + +#include "base/logging.h" +#include "build/build_config.h" + +namespace crashpad { +namespace test { + +namespace { + +bool IsTestDataRoot(const base::FilePath& candidate) { + const base::FilePath marker_path = + candidate.Append(FILE_PATH_LITERAL("util")) + .Append(FILE_PATH_LITERAL("test")) + .Append(FILE_PATH_LITERAL("paths_test_data_root.txt")); + +#if !defined(OS_WIN) + struct stat stat_buf; + int rv = stat(marker_path.value().c_str(), &stat_buf); +#else + struct _stat stat_buf; + int rv = _wstat(marker_path.value().c_str(), &stat_buf); +#endif + + return rv == 0; +} + +base::FilePath TestDataRootInternal() { +#if !defined(OS_WIN) + const char* environment_value = getenv("CRASHPAD_TEST_DATA_ROOT"); +#else // defined(OS_WIN) + const wchar_t* environment_value = _wgetenv(L"CRASHPAD_TEST_DATA_ROOT"); +#endif + + if (environment_value) { + // It was specified explicitly, so use it even if it seems incorrect. + if (!IsTestDataRoot(base::FilePath(environment_value))) { + LOG(WARNING) << "CRASHPAD_TEST_DATA_ROOT seems invalid, honoring anyway"; + } + + return base::FilePath(environment_value); + } + + // In a standalone build, the test executable is usually at + // out/{Debug,Release} relative to the Crashpad root. + const base::FilePath executable = Paths::Executable(); + base::FilePath candidate = + base::FilePath(executable.DirName() + .Append(base::FilePath::kParentDirectory) + .Append(base::FilePath::kParentDirectory)); + if (IsTestDataRoot(candidate)) { + return candidate; + } + + // In an in-Chromium build, the test executable is usually at + // out/{Debug,Release} relative to the Chromium root, and the Crashpad root is + // at third_party/crashpad/crashpad relative to the Chromium root. + candidate = candidate.Append(FILE_PATH_LITERAL("third_party")) + .Append(FILE_PATH_LITERAL("crashpad")) + .Append(FILE_PATH_LITERAL("crashpad")); + if (IsTestDataRoot(candidate)) { + return candidate; + } + + // If nothing else worked, use the current directory, issuing a warning if it + // doesn’t seem right. + if (!IsTestDataRoot(base::FilePath(base::FilePath::kCurrentDirectory))) { + LOG(WARNING) << "could not locate a valid test data root"; + } + + return base::FilePath(base::FilePath::kCurrentDirectory); +} + +} // namespace + +// static +base::FilePath Paths::TestDataRoot() { + static base::FilePath* test_data_root = + new base::FilePath(TestDataRootInternal()); + return *test_data_root; +} + +} // namespace test +} // namespace crashpad diff --git a/util/test/paths.h b/util/test/paths.h new file mode 100644 index 00000000..844413d6 --- /dev/null +++ b/util/test/paths.h @@ -0,0 +1,49 @@ +// Copyright 2015 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. + +#ifndef CRASHPAD_UTIL_TEST_PATHS_H_ +#define CRASHPAD_UTIL_TEST_PATHS_H_ + +#include "base/basictypes.h" +#include "base/files/file_path.h" + +namespace crashpad { +namespace test { + +//! \brief Functions to obtain paths from within tests. +class Paths { + public: + //! \brief Returns the pathname of the currently-running test executable. + static base::FilePath Executable(); + + //! \brief Returns the pathname of the test data root. + //! + //! If the `CRASHPAD_TEST_DATA_ROOT` environment variable is set, its value + //! will be returned. Otherwise, this function will attempt to locate the test + //! data root relative to the executable path. If this fails, it will fall + //! back to returning the current working directory. + //! + //! At present, the test data root is normally the root of the Crashpad source + //! tree, although this may not be the case indefinitely. This function may + //! only be used to locate test data, not for arbitrary access to source + //! files. + static base::FilePath TestDataRoot(); + + DISALLOW_IMPLICIT_CONSTRUCTORS(Paths); +}; + +} // namespace test +} // namespace crashpad + +#endif // CRASHPAD_UTIL_TEST_PATHS_H_ diff --git a/util/test/executable_path_mac.cc b/util/test/paths_mac.cc similarity index 93% rename from util/test/executable_path_mac.cc rename to util/test/paths_mac.cc index e768715a..2d3d84ac 100644 --- a/util/test/executable_path_mac.cc +++ b/util/test/paths_mac.cc @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "util/test/executable_path.h" +#include "util/test/paths.h" #include #include @@ -22,7 +22,8 @@ namespace crashpad { namespace test { -base::FilePath ExecutablePath() { +// static +base::FilePath Paths::Executable() { uint32_t executable_length = 0; _NSGetExecutablePath(nullptr, &executable_length); DCHECK_GT(executable_length, 1u); diff --git a/util/test/executable_path_test.cc b/util/test/paths_test.cc similarity index 69% rename from util/test/executable_path_test.cc rename to util/test/paths_test.cc index 5c18c45b..466e8b68 100644 --- a/util/test/executable_path_test.cc +++ b/util/test/paths_test.cc @@ -12,18 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "util/test/executable_path.h" +#include "util/test/paths.h" #include "base/files/file_path.h" #include "build/build_config.h" #include "gtest/gtest.h" +#include "util/file/file_io.h" namespace crashpad { namespace test { namespace { -TEST(ExecutablePath, ExecutablePath) { - base::FilePath executable_path = ExecutablePath(); +TEST(Paths, Executable) { + base::FilePath executable_path = Paths::Executable(); base::FilePath executable_name = executable_path.BaseName(); #if defined(OS_WIN) EXPECT_EQ(FILE_PATH_LITERAL("crashpad_util_test.exe"), @@ -33,6 +34,15 @@ TEST(ExecutablePath, ExecutablePath) { #endif // OS_WIN } +TEST(Paths, TestDataRoot) { + base::FilePath test_data_root = Paths::TestDataRoot(); + ScopedFileHandle file(LoggingOpenFileForRead( + test_data_root.Append(FILE_PATH_LITERAL("util")) + .Append(FILE_PATH_LITERAL("test")) + .Append(FILE_PATH_LITERAL("paths_test_data_root.txt")))); + EXPECT_TRUE(file.is_valid()); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/util/test/paths_test_data_root.txt b/util/test/paths_test_data_root.txt new file mode 100644 index 00000000..1ddd2642 --- /dev/null +++ b/util/test/paths_test_data_root.txt @@ -0,0 +1,16 @@ +# Copyright 2015 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. + +This file is used by Paths::TestDataRoot() to locate the test data root. It +is present at a known path from the test data root. diff --git a/util/test/executable_path_win.cc b/util/test/paths_win.cc similarity index 92% rename from util/test/executable_path_win.cc rename to util/test/paths_win.cc index 024478d7..bc299a42 100644 --- a/util/test/executable_path_win.cc +++ b/util/test/paths_win.cc @@ -12,14 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "util/test/executable_path.h" +#include "util/test/paths.h" #include namespace crashpad { namespace test { -base::FilePath ExecutablePath() { +// static +base::FilePath Paths::Executable() { wchar_t executable_path[_MAX_PATH]; GetModuleFileName(nullptr, executable_path, sizeof(executable_path)); return base::FilePath(executable_path); diff --git a/util/util.gyp b/util/util.gyp index ac7fc477..f432267d 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -224,9 +224,6 @@ 'sources': [ 'test/errors.cc', 'test/errors.h', - 'test/executable_path.h', - 'test/executable_path_mac.cc', - 'test/executable_path_win.cc', 'test/mac/dyld.h', 'test/mac/mach_errors.cc', 'test/mac/mach_errors.h', @@ -237,6 +234,10 @@ 'test/multiprocess_exec_posix.cc', 'test/multiprocess_exec_win.cc', 'test/multiprocess_posix.cc', + 'test/paths.cc', + 'test/paths.h', + 'test/paths_mac.cc', + 'test/paths_win.cc', 'test/scoped_temp_dir.cc', 'test/scoped_temp_dir.h', 'test/scoped_temp_dir_posix.cc', @@ -308,10 +309,10 @@ 'stdlib/strlcpy_test.cc', 'stdlib/strnlen_test.cc', 'synchronization/semaphore_test.cc', - 'test/executable_path_test.cc', 'test/mac/mach_multiprocess_test.cc', 'test/multiprocess_exec_test.cc', 'test/multiprocess_posix_test.cc', + 'test/paths_test.cc', 'test/scoped_temp_dir_test.cc', 'win/process_info_test.cc', 'win/time_test.cc', diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 52f9fdd9..5b1b7995 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -21,7 +21,7 @@ #include "build/build_config.h" #include "gtest/gtest.h" #include "util/misc/uuid.h" -#include "util/test/executable_path.h" +#include "util/test/paths.h" #include "util/win/scoped_handle.h" namespace crashpad { @@ -77,7 +77,7 @@ TEST(ProcessInfo, SomeOtherProcess) { CreateEvent(nullptr, true, false, done_uuid.ToString16().c_str())); ASSERT_TRUE(done.get()); - base::FilePath test_executable = ExecutablePath(); + base::FilePath test_executable = Paths::Executable(); std::wstring child_test_executable = test_executable.RemoveFinalExtension().value() + L"_process_info_test_child.exe";