Skip to content

Commit b0a9014

Browse files
jmarantzhtuch
jmarantz
authored and
htuch
committed
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 <[email protected]>
1 parent 8567460 commit b0a9014

File tree

4 files changed

+33
-0
lines changed

4 files changed

+33
-0
lines changed

‎tools/check_format.py‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,15 @@
2323
GOOGLE_PROTOBUF_WHITELIST = ("ci/prebuilt", "source/common/protobuf", "api/test")
2424
REPOSITORIES_BZL = "bazel/repositories.bzl"
2525

26+
# Files matching these exact names can reference prod time. These include the class
27+
# definitions for prod time, the construction of them in main(), and perf annotation.
28+
# For now it includes the validation server but that really should be injected too.
29+
PROD_TIME_WHITELIST = ('./source/common/common/utility.h',
30+
'./source/exe/main_common.cc',
31+
'./source/exe/main_common.h',
32+
'./source/server/config_validation/server.cc',
33+
'./source/common/common/perf_annotation.h')
34+
2635
CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-6.0")
2736
BUILDIFIER_PATH = os.getenv("BUILDIFIER_BIN", "$GOPATH/bin/buildifier")
2837
ENVOY_BUILD_FIXER_PATH = os.path.join(
@@ -65,6 +74,12 @@ def whitelistedForProtobufDeps(file_path):
6574
return (file_path.endswith(PROTO_SUFFIX) or file_path.endswith(REPOSITORIES_BZL) or \
6675
any(path_segment in file_path for path_segment in GOOGLE_PROTOBUF_WHITELIST))
6776

77+
# Production time sources should not be instantiated in the source, except for a few
78+
# specific cases. They should be passed down from where they are instantied to where
79+
# they need to be used, e.g. through the ServerInstance, Dispatcher, or ClusterManager.
80+
def whitelistedForProdTime(file_path):
81+
return file_path in PROD_TIME_WHITELIST or file_path.startswith('./test/')
82+
6883
def findSubstringAndReturnError(pattern, file_path, error_message):
6984
with open(file_path) as f:
7085
text = f.read()
@@ -145,6 +160,9 @@ def checkSourceLine(line, file_path, reportError):
145160
# comments, for example this one.
146161
reportError("Don't use <mutex> or <condition_variable*>, switch to "
147162
"Thread::MutexBasicLockable in source/common/common/thread.h")
163+
if not whitelistedForProdTime(file_path):
164+
if 'ProdSystemTimeSource' in line or 'ProdMonotonicTimeSource' in line:
165+
reportError("Don't reference real-time sources from production code; use injection")
148166

149167
def checkBuildLine(line, file_path, reportError):
150168
if not whitelistedForProtobufDeps(file_path) and '"protobuf"' in line:

‎tools/check_format_test_helper.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,10 @@ def checkFileExpectingOK(filename):
143143
"Don't use <mutex> or <condition_variable*>")
144144
errors += fixFileExpectingFailure("condition_variable_any.cc",
145145
"Don't use <mutex> or <condition_variable*>")
146+
errors += fixFileExpectingFailure(
147+
"prod_system_time.cc", "Don't reference real-time sources from production code; use injection")
148+
errors += fixFileExpectingFailure(
149+
"prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection")
146150

147151
errors += fixFileExpectingNoChange("ok_file.cc")
148152

@@ -163,6 +167,10 @@ def checkFileExpectingOK(filename):
163167
errors += checkFileExpectingError("bad_envoy_build_sys_ref.BUILD",
164168
"Superfluous '@envoy//' prefix")
165169
errors += checkFileExpectingError("proto_format.proto", "clang-format check failed")
170+
errors += checkFileExpectingError(
171+
"prod_system_time.cc", "Don't reference real-time sources from production code; use injection")
172+
errors += checkFileExpectingError(
173+
"prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection")
166174

167175
errors += checkFileExpectingOK("ok_file.cc")
168176

‎tools/testdata/check_format/prod_monotonic_time.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
int foo() {
2+
ProdSystemTimeSource system_time;
3+
ProdMonotonicTimeSource monotonic_time;
4+
}

‎tools/testdata/check_format/prod_system_time.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
int foo() {
2+
ProdMonotonicTimeSource monotonic_time;
3+
}

0 commit comments

Comments
 (0)