Skip to content

Commit

Permalink
test: adding an option to hard-fail when deprecated config is used.
Browse files Browse the repository at this point in the history
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
  • Loading branch information
alyssawilk committed Aug 19, 2019
1 parent 817b2e3 commit 9fae5a9
Show file tree
Hide file tree
Showing 8 changed files with 32 additions and 2 deletions.
6 changes: 5 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,11 @@ maximize the chances of your PR being merged.
could convert from the earlier API to the new API. A field may be deprecated
if this tool would be able to perform the conversion. For example, removing a
field to describe HTTP/2 window settings is valid if a more comprehensive
HTTP/2 protocol options field is being introduced to replace it.
HTTP/2 protocol options field is being introduced to replace it. The PR author
deprecating the old configuration is responsible for updating all tests and
canonical configuration, or guarding them with ifndef DISABLE_DEPRECATED_FEATURES.
This will be validated by the bazel.debug target, which will hard-fail when
deprecated configuration is used.
* For configuration deprecations that are not covered by the above semantic
replacement policy, any deprecation will only take place after
community consultation on mailing lists, Slack and GitHub, over the period of
Expand Down
5 changes: 5 additions & 0 deletions bazel/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ config_setting(
values = {"define": "object_dump_on_signal_trace=disabled"},
)

config_setting(
name = "disable_deprecated_features",
values = {"define": "deprecated_features=disabled"},
)

config_setting(
name = "disable_hot_restart",
values = {"define": "hot_restart=disabled"},
Expand Down
2 changes: 2 additions & 0 deletions bazel/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ The following optional features can be disabled on the Bazel build command-line:
* Backtracing on signals with `--define signal_trace=disabled`
* Active stream state dump on signals with `--define signal_trace=disabled` or `--define disable_object_dump_on_signal_trace=disabled`
* tcmalloc with `--define tcmalloc=disabled`
* deprecated features with `--define deprecated_features=disabled`


## Enabling optional features

Expand Down
3 changes: 3 additions & 0 deletions bazel/envoy_internal.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ def envoy_copts(repository, test = False):
}) + select({
repository + "//bazel:disable_object_dump_on_signal_trace": [],
"//conditions:default": ["-DENVOY_OBJECT_TRACE_ON_DUMP"],
}) + select({
repository + "//bazel:disable_deprecated_features": ["-DDISABLE_DEPRECATED_FEATURES"],
"//conditions:default": [],
}) + select({
repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"],
"//conditions:default": [],
Expand Down
2 changes: 1 addition & 1 deletion ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ elif [[ "$CI_TARGET" == "bazel.debug" ]]; then
echo "bazel debug build with tests..."
bazel_binary_build debug
echo "Testing ${TEST_TARGETS}"
bazel test ${BAZEL_BUILD_OPTIONS} -c dbg ${TEST_TARGETS}
bazel test ${BAZEL_BUILD_OPTIONS} --define deprecated_features=disabled -c dbg ${TEST_TARGETS}
exit 0
elif [[ "$CI_TARGET" == "bazel.debug.server_only" ]]; then
setup_clang_toolchain
Expand Down
5 changes: 5 additions & 0 deletions source/common/runtime/runtime_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,14 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const {
// If either disallowed by default or configured off, the feature is not enabled.
return false;
}

// The feature is allowed. It is assumed this check is called when the feature
// is about to be used, so increment the feature use stat.
stats_.deprecated_feature_use_.inc();
#ifdef DISABLE_DEPRECATED_FEATURES
return false;
#endif

return true;
}

Expand Down
2 changes: 2 additions & 0 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) {
EXPECT_EQ(0, runtime_deprecated_feature_use_.value());
}

#ifndef DISABLE_DEPRECATED_FEATURES
TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) {
envoy::test::deprecation_test::Base base;
base.set_is_deprecated("foo");
Expand Down Expand Up @@ -617,6 +618,7 @@ TEST_F(DeprecatedFieldsTest, RepeatedMessageDeprecated) {
"'envoy.test.deprecation_test.Base.deprecated_repeated_message'",
MessageUtil::checkForDeprecation(base));
}
#endif

class TimestampUtilTest : public testing::Test, public ::testing::WithParamInterface<int64_t> {};

Expand Down
9 changes: 9 additions & 0 deletions test/common/runtime/runtime_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ TEST_F(DiskLoaderImplTest, All) {
// test_feature_false is not in runtime_features.cc and so is false by default.
EXPECT_EQ(false, snapshot->runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false"));

// Deprecation
#ifdef DISABLE_DEPRECATED_FEATURES
EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled"));
#else
EXPECT_EQ(true, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled"));
#endif
EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled(
"envoy.deprecated_features.deprecated.proto:is_deprecated_fatal"));

// Feature defaults via helper function.
EXPECT_EQ(false, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false"));
EXPECT_EQ(true, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_true"));
Expand Down

0 comments on commit 9fae5a9

Please sign in to comment.