From b0a901480c5933ddfab99e0071ebfe75ac8a0b18 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 26 Aug 2018 12:56:00 -0400 Subject: [PATCH] time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (#4248) Time should be injected in all production code, so that it can ultimately be tested using mock time or simulated time. Production code that directly instantiates ProdSystemTimeSource makes this impossible, and this PR ensures that 'format' checks fail if that is violated. This partially addresses issue #4160. Risk Level: low Testing: //test/... Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz --- tools/check_format.py | 18 ++++++++++++++++++ tools/check_format_test_helper.py | 8 ++++++++ .../check_format/prod_monotonic_time.cc | 4 ++++ .../testdata/check_format/prod_system_time.cc | 3 +++ 4 files changed, 33 insertions(+) create mode 100644 tools/testdata/check_format/prod_monotonic_time.cc create mode 100644 tools/testdata/check_format/prod_system_time.cc diff --git a/tools/check_format.py b/tools/check_format.py index c9e8fc243535..6f643cd6b7f9 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -23,6 +23,15 @@ GOOGLE_PROTOBUF_WHITELIST = ("ci/prebuilt", "source/common/protobuf", "api/test") REPOSITORIES_BZL = "bazel/repositories.bzl" +# Files matching these exact names can reference prod time. These include the class +# definitions for prod time, the construction of them in main(), and perf annotation. +# For now it includes the validation server but that really should be injected too. +PROD_TIME_WHITELIST = ('./source/common/common/utility.h', + './source/exe/main_common.cc', + './source/exe/main_common.h', + './source/server/config_validation/server.cc', + './source/common/common/perf_annotation.h') + CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-6.0") BUILDIFIER_PATH = os.getenv("BUILDIFIER_BIN", "$GOPATH/bin/buildifier") ENVOY_BUILD_FIXER_PATH = os.path.join( @@ -65,6 +74,12 @@ def whitelistedForProtobufDeps(file_path): return (file_path.endswith(PROTO_SUFFIX) or file_path.endswith(REPOSITORIES_BZL) or \ any(path_segment in file_path for path_segment in GOOGLE_PROTOBUF_WHITELIST)) +# Production time sources should not be instantiated in the source, except for a few +# specific cases. They should be passed down from where they are instantied to where +# they need to be used, e.g. through the ServerInstance, Dispatcher, or ClusterManager. +def whitelistedForProdTime(file_path): + return file_path in PROD_TIME_WHITELIST or file_path.startswith('./test/') + def findSubstringAndReturnError(pattern, file_path, error_message): with open(file_path) as f: text = f.read() @@ -145,6 +160,9 @@ def checkSourceLine(line, file_path, reportError): # comments, for example this one. reportError("Don't use or , switch to " "Thread::MutexBasicLockable in source/common/common/thread.h") + if not whitelistedForProdTime(file_path): + if 'ProdSystemTimeSource' in line or 'ProdMonotonicTimeSource' in line: + reportError("Don't reference real-time sources from production code; use injection") def checkBuildLine(line, file_path, reportError): if not whitelistedForProtobufDeps(file_path) and '"protobuf"' in line: diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 17ad7edf9f58..2d7b02081a34 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -143,6 +143,10 @@ def checkFileExpectingOK(filename): "Don't use or ") errors += fixFileExpectingFailure("condition_variable_any.cc", "Don't use or ") + errors += fixFileExpectingFailure( + "prod_system_time.cc", "Don't reference real-time sources from production code; use injection") + errors += fixFileExpectingFailure( + "prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection") errors += fixFileExpectingNoChange("ok_file.cc") @@ -163,6 +167,10 @@ def checkFileExpectingOK(filename): errors += checkFileExpectingError("bad_envoy_build_sys_ref.BUILD", "Superfluous '@envoy//' prefix") errors += checkFileExpectingError("proto_format.proto", "clang-format check failed") + errors += checkFileExpectingError( + "prod_system_time.cc", "Don't reference real-time sources from production code; use injection") + errors += checkFileExpectingError( + "prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection") errors += checkFileExpectingOK("ok_file.cc") diff --git a/tools/testdata/check_format/prod_monotonic_time.cc b/tools/testdata/check_format/prod_monotonic_time.cc new file mode 100644 index 000000000000..10bcfc7c5d3e --- /dev/null +++ b/tools/testdata/check_format/prod_monotonic_time.cc @@ -0,0 +1,4 @@ +int foo() { + ProdSystemTimeSource system_time; + ProdMonotonicTimeSource monotonic_time; +} diff --git a/tools/testdata/check_format/prod_system_time.cc b/tools/testdata/check_format/prod_system_time.cc new file mode 100644 index 000000000000..a533661b9805 --- /dev/null +++ b/tools/testdata/check_format/prod_system_time.cc @@ -0,0 +1,3 @@ +int foo() { + ProdMonotonicTimeSource monotonic_time; +}