Fix flags_test flakiness
The tests could run in parallel and step on eachothers logic by
setting/resetting global properties.
Revamp the logic to generate unique property names using scratch
files to prevent collisions.
Test: gtest
Bug: 190150217
Bug: 181748174
Change-Id: If870478e13dcfa2487da57b223b322847e702ea2
diff --git a/libartbase/base/flags.cc b/libartbase/base/flags.cc
index 46de9c0..4b7e780 100644
--- a/libartbase/base/flags.cc
+++ b/libartbase/base/flags.cc
@@ -94,6 +94,11 @@
}
template <typename Value>
+Flag<Value>::~Flag() {
+ ALL_FLAGS.remove(this);
+}
+
+template <typename Value>
void Flag<Value>::Reload() {
// The cmdline flags are loaded by the parsed_options infra.
diff --git a/libartbase/base/flags.h b/libartbase/base/flags.h
index ae166e6..973a5da 100644
--- a/libartbase/base/flags.h
+++ b/libartbase/base/flags.h
@@ -140,7 +140,7 @@
// Create a new Flag. The name parameter is used to generate the names from the various parameter
// sources. See the documentation on the Flags struct for an example.
explicit Flag(const std::string& name, Value default_value = {});
- virtual ~Flag() {}
+ virtual ~Flag();
// Returns the flag value.
@@ -190,7 +190,7 @@
std::optional<Value> from_system_property_;
std::optional<Value> from_server_setting_;
- friend class FlagsTests;
+ friend class TestFlag;
};
// This struct contains the list of ART flags. Flags are parameterized by the type of value they
diff --git a/libartbase/base/flags_test.cc b/libartbase/base/flags_test.cc
index cfbf2a9..22f5f5a 100644
--- a/libartbase/base/flags_test.cc
+++ b/libartbase/base/flags_test.cc
@@ -24,45 +24,110 @@
namespace art {
+// Tests may be run in parallel so this helper class ensures
+// that we generate a unique test flag each time to avoid
+// tests stepping on each other
+class TestFlag {
+ public:
+ // Takes control of the tmp_file pointer.
+ explicit TestFlag(ScratchFile* tmp_file) {
+ tmp_file_.reset(tmp_file);
+
+ std::string tmp_name = tmp_file_->GetFilename();
+ size_t tmp_last_slash = tmp_name.rfind('/');
+ tmp_name = tmp_name.substr(tmp_last_slash + 1);
+
+ flag_name_ = "art.gtest." + tmp_name;
+ system_prop_name_ = "dalvik.vm." + flag_name_;
+ server_name_ = "persist.device_config.runtime_native." + flag_name_;
+ cmd_line_name_ = flag_name_;
+ std::replace(cmd_line_name_.begin(), cmd_line_name_.end(), '.', '-');
+
+ flag_.reset(new Flag<int>(flag_name_, /*default_value=*/ 42));
+ }
+
+ void AssertCmdlineValue(bool has_value, int expected) {
+ ASSERT_EQ(flag_->from_command_line_.has_value(), has_value);
+ if (has_value) {
+ ASSERT_EQ(flag_->from_command_line_.value(), expected);
+ }
+ }
+
+ void AssertSysPropValue(bool has_value, int expected) {
+ ASSERT_EQ(flag_->from_system_property_.has_value(), has_value);
+ if (has_value) {
+ ASSERT_EQ(flag_->from_system_property_.value(), expected);
+ }
+ }
+
+ void AssertServerSettingValue(bool has_value, int expected) {
+ ASSERT_EQ(flag_->from_server_setting_.has_value(), has_value);
+ if (has_value) {
+ ASSERT_EQ(flag_->from_server_setting_.value(), expected);
+ }
+ }
+
+ void AssertDefaultValue(int expected) {
+ ASSERT_EQ(flag_->default_, expected);
+ }
+
+ int Value() {
+ return (*flag_)();
+ }
+
+ std::string SystemProperty() const {
+ return system_prop_name_;
+ }
+
+ std::string ServerSetting() const {
+ return server_name_;
+ }
+
+ std::string CmdLineName() const {
+ return cmd_line_name_;
+ }
+
+ private:
+ std::unique_ptr<ScratchFile> tmp_file_;
+ std::unique_ptr<Flag<int>> flag_;
+ std::string flag_name_;
+ std::string cmd_line_name_;
+ std::string system_prop_name_;
+ std::string server_name_;
+};
+
class FlagsTests : public CommonRuntimeTest {
protected:
- void assertCmdlineValue(bool has_value, int expected) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.from_command_line_.has_value(), has_value);
- if (has_value) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.from_command_line_.value(), expected);
- }
+ // We need to initialize the flag after the ScratchDir is created
+ // but before we configure the runtime options (so that we can get
+ // the right name for the config).
+ //
+ // So we do it in SetUpRuntimeOptions.
+ virtual void SetUpRuntimeOptions(RuntimeOptions* options) {
+ test_flag_.reset(new TestFlag(new ScratchFile()));
+ CommonRuntimeTest::SetUpRuntimeOptions(options);
}
- void assertSysPropValue(bool has_value, int expected) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.from_system_property_.has_value(), has_value);
- if (has_value) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.from_system_property_.value(), expected);
- }
+ virtual void TearDown() {
+ test_flag_ = nullptr;
+ CommonRuntimeTest::TearDown();
}
- void assertServerSettingValue(bool has_value, int expected) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.from_server_setting_.has_value(), has_value);
- if (has_value) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.from_server_setting_.value(), expected);
- }
- }
-
- void assertDefaultValue(int expected) {
- ASSERT_EQ(gFlags.MyFeatureTestFlag.default_, expected);
- }
+ std::unique_ptr<TestFlag> test_flag_;
};
class FlagsTestsWithCmdLine : public FlagsTests {
- public:
- ~FlagsTestsWithCmdLine() {
- android::base::SetProperty("dalvik.vm.my-feature-test.flag", "");
- android::base::SetProperty("persist.device_config.runtime_native.my-feature-test.flag", "");
+ protected:
+ virtual void TearDown() {
+ // android::base::SetProperty(test_flag_->SystemProperty(), "");
+ android::base::SetProperty(test_flag_->ServerSetting(), "");
+ FlagsTests::TearDown();
}
- protected:
- void SetUpRuntimeOptions(RuntimeOptions* options) override {
- // Disable implicit dex2oat invocations when loading image spaces.
- options->emplace_back("-Xmy-feature-test-flag:1", nullptr);
+ virtual void SetUpRuntimeOptions(RuntimeOptions* options) {
+ test_flag_.reset(new TestFlag(new ScratchFile()));
+ std::string option = "-X" + test_flag_->CmdLineName() + ":1";
+ options->emplace_back(option.c_str(), nullptr);
}
};
@@ -71,53 +136,53 @@
TEST_F(FlagsTests, ValidateDefaultValue) {
FlagBase::ReloadAllFlags("test");
- assertCmdlineValue(false, 1);
- assertSysPropValue(false, 2);
- assertServerSettingValue(false, 3);
- assertDefaultValue(42);
+ test_flag_->AssertCmdlineValue(false, 1);
+ test_flag_->AssertSysPropValue(false, 2);
+ test_flag_->AssertServerSettingValue(false, 3);
+ test_flag_->AssertDefaultValue(42);
- ASSERT_EQ(gFlags.MyFeatureTestFlag(), 42);
+ ASSERT_EQ(test_flag_->Value(), 42);
}
// Validate that the server side config is picked when it is set.
TEST_F(FlagsTestsWithCmdLine, FlagsTestsGetValueServerSetting) {
- android::base::SetProperty("dalvik.vm.my-feature-test.flag", "2");
- android::base::SetProperty("persist.device_config.runtime_native.my-feature-test.flag", "3");
+ ASSERT_TRUE(android::base::SetProperty(test_flag_->SystemProperty(), "2"));
+ ASSERT_TRUE(android::base::SetProperty(test_flag_->ServerSetting(), "3"));
FlagBase::ReloadAllFlags("test");
- assertCmdlineValue(true, 1);
- assertSysPropValue(true, 2);
- assertServerSettingValue(true, 3);
- assertDefaultValue(42);
+ test_flag_->AssertCmdlineValue(true, 1);
+ test_flag_->AssertSysPropValue(true, 2);
+ test_flag_->AssertServerSettingValue(true, 3);
+ test_flag_->AssertDefaultValue(42);
- ASSERT_EQ(gFlags.MyFeatureTestFlag(), 3);
+ ASSERT_EQ(test_flag_->Value(), 3);
}
// Validate that the system property value is picked when the server one is not set.
TEST_F(FlagsTestsWithCmdLine, FlagsTestsGetValueSysProperty) {
- android::base::SetProperty("dalvik.vm.my-feature-test.flag", "2");
+ ASSERT_TRUE(android::base::SetProperty(test_flag_->SystemProperty(), "2"));
FlagBase::ReloadAllFlags("test");
- assertCmdlineValue(true, 1);
- assertSysPropValue(true, 2);
- assertServerSettingValue(false, 3);
- assertDefaultValue(42);
+ test_flag_->AssertCmdlineValue(true, 1);
+ test_flag_->AssertSysPropValue(true, 2);
+ test_flag_->AssertServerSettingValue(false, 3);
+ test_flag_->AssertDefaultValue(42);
- ASSERT_EQ(gFlags.MyFeatureTestFlag(), 2);
+ ASSERT_EQ(test_flag_->Value(), 2);
}
// Validate that the cmdline value is picked when no properties are set.
TEST_F(FlagsTestsWithCmdLine, FlagsTestsGetValueCmdline) {
FlagBase::ReloadAllFlags("test");
- assertCmdlineValue(true, 1);
- assertSysPropValue(false, 2);
- assertServerSettingValue(false, 3);
- assertDefaultValue(42);
+ test_flag_->AssertCmdlineValue(true, 1);
+ test_flag_->AssertSysPropValue(false, 2);
+ test_flag_->AssertServerSettingValue(false, 3);
+ test_flag_->AssertDefaultValue(42);
- ASSERT_EQ(gFlags.MyFeatureTestFlag(), 1);
+ ASSERT_EQ(test_flag_->Value(), 1);
}
} // namespace art