From be869c313c88fbd13002ed733cafc4020aa7abf4 Mon Sep 17 00:00:00 2001 From: divyagayathri-hcl <159437886+divyagayathri-hcl@users.noreply.github.com> Date: Mon, 22 Jul 2024 07:53:55 -0700 Subject: [PATCH] [Thinkit] Replaced GetGenericTestbed with GetTestbedWithRequirements to take in TestbedRequiredments proto. (#359) [ThinKit] Fix PacketGenerationFinalizer and RequestType enum. [thinkit] Benchmark performance of Bazel Test Environment. [thinkit] Add a way to set test_case_id with Bazel Test Environment. [thinkit] Add a mutex to BazelTestEnvironment making it thread safe. [thinkit] Language change to remove disrespectful term. [thinkit] Add a test for BazelTestEnvironment using protos. Co-authored-by: rhalstea Co-authored-by: bhagatgit <115375369+bhagatgit@users.noreply.github.com> --- pins_infra_deps.bzl | 7 ++ thinkit/BUILD.bazel | 14 ++- thinkit/bazel_test_environment.cc | 5 +- thinkit/bazel_test_environment.h | 30 ++++-- thinkit/bazel_test_environment_test.cc | 122 ++++++++++++++++++++++--- thinkit/generic_testbed.h | 5 +- thinkit/generic_testbed_fixture.h | 12 ++- thinkit/packet_generation_finalizer.h | 2 - thinkit/proto/generic_testbed.proto | 2 +- 9 files changed, 170 insertions(+), 29 deletions(-) diff --git a/pins_infra_deps.bzl b/pins_infra_deps.bzl index ff1ff6b9..2bedc86b 100644 --- a/pins_infra_deps.bzl +++ b/pins_infra_deps.bzl @@ -66,6 +66,13 @@ def pins_infra_deps(): strip_prefix = "benchmark-1.5.4", sha256 = "e3adf8c98bb38a198822725c0fc6c0ae4711f16fbbf6aeb311d5ad11e5a081b5", ) + if not native.existing_rule("com_google_benchmark"): + http_archive( + name = "com_google_benchmark", + urls = ["https://github.com/google/benchmark/archive/v1.5.4.tar.gz"], + strip_prefix = "benchmark-1.5.4", + sha256 = "e3adf8c98bb38a198822725c0fc6c0ae4711f16fbbf6aeb311d5ad11e5a081b5", + ) if not native.existing_rule("com_google_protobuf"): http_archive( name = "com_google_protobuf", diff --git a/thinkit/BUILD.bazel b/thinkit/BUILD.bazel index 651aa239..093a7ceb 100644 --- a/thinkit/BUILD.bazel +++ b/thinkit/BUILD.bazel @@ -170,6 +170,7 @@ cc_library( "//gutil:status", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", + "@com_google_absl//absl/synchronization", "@com_google_googletest//:gtest", ], ) @@ -179,9 +180,16 @@ cc_test( srcs = ["bazel_test_environment_test.cc"], deps = [ ":bazel_test_environment", - "//gutil:status_matchers", - "@com_google_absl//absl/strings", + ":test_environment", + "@com_google_protobuf//:protobuf", "@com_google_googletest//:gtest_main", + "@com_google_absl//absl/memory", + "@com_google_absl//absl/strings", + # This dependency is not technically needed inside Google3, + # but is a hack to keep the Copybara rules simple. + "@com_google_benchmark//:benchmark", + "//gutil:status_matchers", + "//gutil:testing", ], ) @@ -239,7 +247,9 @@ cc_library( hdrs = ["generic_testbed_fixture.h"], deps = [ ":generic_testbed", + "//thinkit/proto:generic_testbed_cc_proto", "@com_google_absl//absl/memory", + "@com_google_absl//absl/status:statusor", "@com_google_googletest//:gtest", ], ) diff --git a/thinkit/bazel_test_environment.cc b/thinkit/bazel_test_environment.cc index b8d22eac..28d5c1eb 100644 --- a/thinkit/bazel_test_environment.cc +++ b/thinkit/bazel_test_environment.cc @@ -24,6 +24,7 @@ #include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "absl/synchronization/mutex.h" #include "gtest/gtest.h" #include "gutil/status.h" #include "thinkit/test_environment.h" @@ -34,7 +35,7 @@ namespace { absl::StatusOr ArtifactDirectory() { // Pick appropriate artifact directory using Bazel environment variables, see - // https://docs.bazel.build/versions/master/test-encyclopedia.html#initial-conditions + // https://docs.bazel.build/versions/main/test-encyclopedia.html#initial-conditions char* base_dir = std::getenv("TEST_UNDECLARED_OUTPUTS_DIR"); if (base_dir == nullptr) { base_dir = std::getenv("TEST_TMPDIR"); @@ -84,11 +85,13 @@ absl::Status WriteToTestArtifact(absl::string_view filename, absl::Status BazelTestEnvironment::StoreTestArtifact( absl::string_view filename, absl::string_view contents) { + absl::MutexLock lock(&this->write_mutex_); return WriteToTestArtifact(filename, contents, std::ios_base::trunc); } absl::Status BazelTestEnvironment::AppendToTestArtifact( absl::string_view filename, absl::string_view contents) { + absl::MutexLock lock(&this->write_mutex_); return WriteToTestArtifact(filename, contents, std::ios_base::app); } diff --git a/thinkit/bazel_test_environment.h b/thinkit/bazel_test_environment.h index b0b788b7..8f47f2d3 100644 --- a/thinkit/bazel_test_environment.h +++ b/thinkit/bazel_test_environment.h @@ -18,30 +18,48 @@ #include #include "absl/strings/string_view.h" +#include "absl/synchronization/mutex.h" #include "thinkit/test_environment.h" namespace thinkit { // Simple `thinkit::TestEnvironment` that works well with the Bazel build // system. +// Calls to {Store,AppendTo}TestArtifact within a BazelTestEnvironment +// object are guaranteed to be thread-safe due to writes being sequential. class BazelTestEnvironment : public TestEnvironment { public: BazelTestEnvironment() = delete; - BazelTestEnvironment(bool mask_known_failures) - : mask_known_failures_{mask_known_failures} {} + explicit BazelTestEnvironment( + bool mask_known_failures, + std::function set_test_case_id = [](auto) {}) + : mask_known_failures_{mask_known_failures}, + set_test_case_id_(std::move(set_test_case_id)) {} absl::Status StoreTestArtifact(absl::string_view filename, - absl::string_view contents) override; - using TestEnvironment::StoreTestArtifact; // Inherit protobuf overload. + absl::string_view contents) + ABSL_LOCKS_EXCLUDED(write_mutex_) override; + using TestEnvironment::StoreTestArtifact; // Inherit protobuf overload which + // calls string_view version. absl::Status AppendToTestArtifact(absl::string_view filename, - absl::string_view contents) override; - using TestEnvironment::AppendToTestArtifact; // Inherit protobuf overload. + absl::string_view contents) + ABSL_LOCKS_EXCLUDED(write_mutex_) override; + using TestEnvironment::AppendToTestArtifact; // Inherit protobuf overload + // which calls string_view + // version. bool MaskKnownFailures() { return mask_known_failures_; }; + void SetTestCaseID(absl::string_view test_case_id) override { + set_test_case_id_(test_case_id); + } + private: bool mask_known_failures_; + std::function set_test_case_id_; + // The mutex is used to ensure that writes to disk are sequential. + absl::Mutex write_mutex_; }; } // namespace thinkit diff --git a/thinkit/bazel_test_environment_test.cc b/thinkit/bazel_test_environment_test.cc index bd5e08eb..71659b74 100644 --- a/thinkit/bazel_test_environment_test.cc +++ b/thinkit/bazel_test_environment_test.cc @@ -1,32 +1,130 @@ #include "thinkit/bazel_test_environment.h" +#include +#include +#include + +// Switching benchmark dependency to third_party seems to not output any +// benchmarking information when run. +#include "absl/memory/memory.h" #include "absl/strings/string_view.h" +#include "benchmark/benchmark.h" #include "gmock/gmock.h" +#include "google/protobuf/wrappers.pb.h" #include "gtest/gtest.h" #include "gutil/status_matchers.h" +#include "gutil/testing.h" +#include "thinkit/test_environment.h" namespace thinkit { namespace { +using ::gutil::IsOk; +using ::testing::StrEq; + +// -- Tests -------------------------------------------------------------------- + constexpr absl::string_view kTestArtifact = "my_test_artifact.txt"; -class BazelTestEnvironmentTest : public testing::Test { - protected: - std::unique_ptr environment_ = +TEST(BazelTestEnvironmentTest, StoreTestArtifact) { + std::unique_ptr environment = absl::make_unique(/*mask_known_failures=*/true); -}; - -TEST_F(BazelTestEnvironmentTest, StoreTestArtifact) { - EXPECT_OK(environment_->StoreTestArtifact(kTestArtifact, "Hello, World!\n")); - EXPECT_OK(environment_->StoreTestArtifact(kTestArtifact, "Hello, Test!\n")); + EXPECT_OK(environment->StoreTestArtifact(kTestArtifact, "Hello, World!\n")); + EXPECT_OK(environment->StoreTestArtifact(kTestArtifact, "Hello, Test!\n")); } -TEST_F(BazelTestEnvironmentTest, AppendToTestArtifact) { - EXPECT_OK( - environment_->AppendToTestArtifact(kTestArtifact, "Hello, World!\n")); +TEST(BazelTestEnvironmentTest, AppendToTestArtifact) { + std::unique_ptr environment = + absl::make_unique(/*mask_known_failures=*/true); EXPECT_OK( - environment_->AppendToTestArtifact(kTestArtifact, "Hello, Test!\n")); + environment->AppendToTestArtifact(kTestArtifact, "Hello, World!\n")); + EXPECT_OK(environment->AppendToTestArtifact(kTestArtifact, "Hello, Test!\n")); +} + +TEST(BazelTestEnvironmentTest, + BazelTestEnvironmentStoreAndAppendTestArtifactWithProto) { + // Explicitly uses the BazelTestEnvironment to ensure that we inherit + // definitions from TestEnvironment appropriately. + std::unique_ptr environment = + absl::make_unique(/*mask_known_failures=*/true); + auto foo_proto = gutil::ParseProtoOrDie(R"pb( + value: 42)pb"); + EXPECT_OK(environment->StoreTestArtifact(kTestArtifact, foo_proto)); + EXPECT_OK(environment->AppendToTestArtifact(kTestArtifact, foo_proto)); +} + +// Test that SetTestCaseID correctly calls its input function. +TEST(BazelTestEnvironmentTest, SetTestCaseIdCallsMemberFunction) { + std::string stored_test_case_id; + std::string test_id = "1"; + + std::unique_ptr environment = + absl::make_unique( + /*mask_known_failures=*/true, + /*set_test_case_id=*/[&](absl::string_view test_case_id) { + stored_test_case_id = test_case_id; + }); + + environment->SetTestCaseID(test_id); + EXPECT_THAT(test_id, StrEq(stored_test_case_id)); +} + +// SetTestCaseID should not crash even if the environment is constructed without +// a function. +TEST(BazelTestEnvironmentTest, SetTestCaseIdWorksForUnaryConstructor) { + std::unique_ptr environment = + absl::make_unique(/*mask_known_failures=*/true); + environment->SetTestCaseID("1"); +} + +// -- Benchmarks --------------------------------------------------------------- +// +// Best run with 'blaze test --benchmarks=all'. +// +// Ideally, we would like to use 'benchy' for benchmarking purposes, but, +// because we are benchmarking a testing environment, it relies on being set +// up as a test environment. +// +// For now, one can manually set the environment variable TEST_TMPDIR, then run +// it with benchy, but do not expect that to remain feasible since we may rely +// on additional parts of the test environment in the future. + +// The state specifies the size of the written string in Bytes. +void RunBenchmark(benchmark::State& state, bool truncate) { + const int size = state.range(0); + const std::string filename = "benchmark_file"; + BazelTestEnvironment env(false); + + std::string str(size, 'a'); + if (truncate) { + for (auto s : state) { + ASSERT_THAT(env.StoreTestArtifact(filename, str), IsOk()); + } + } else { + for (auto s : state) { + ASSERT_THAT(env.AppendToTestArtifact(filename, str), IsOk()); + } + } + // Outputs number of iterations (items) per second. + state.SetItemsProcessed(static_cast(state.iterations())); +} + +void BM_Bazel_AppendToTestArtifact(benchmark::State& state) { + RunBenchmark(state, /*truncate=*/false); +} + +BENCHMARK(BM_Bazel_AppendToTestArtifact) + ->Args({/*write_size in bytes=*/1}) + ->Args({1024}) + ->Args({1024 * 1024}); + +void BM_Bazel_StoreTestArtifact(benchmark::State& state) { + RunBenchmark(state, /*truncate=*/true); } +BENCHMARK(BM_Bazel_StoreTestArtifact) + ->Args({/*write_size in bytes=*/1}) + ->Args({1024}) + ->Args({1024 * 1024}); } // namespace } // namespace thinkit diff --git a/thinkit/generic_testbed.h b/thinkit/generic_testbed.h index 4f5f7f3d..d0ad31f4 100644 --- a/thinkit/generic_testbed.h +++ b/thinkit/generic_testbed.h @@ -36,14 +36,14 @@ struct HttpResponse { enum class RequestType { kGet, kPost, - kPut, + kPatch, kDelete, }; // InterfaceInfo represents the mode of an interface and the name of the peer // interface. struct InterfaceInfo { - third_party::pins_infra::thinkit::InterfaceMode interface_mode; + thinkit::InterfaceMode interface_mode; std::string peer_interface_name; // Empty if not applicable. }; @@ -68,6 +68,7 @@ class GenericTestbed { GetSutInterfaceInfo() = 0; // Sends a REST request to the Ixia and returns the response. + // `url` can be either "https://...", "/api/...", or "/ixnetwork/...". virtual absl::StatusOr SendRestRequestToIxia( RequestType type, absl::string_view url, absl::string_view payload) = 0; }; diff --git a/thinkit/generic_testbed_fixture.h b/thinkit/generic_testbed_fixture.h index e28b1d2b..70669974 100644 --- a/thinkit/generic_testbed_fixture.h +++ b/thinkit/generic_testbed_fixture.h @@ -18,8 +18,10 @@ #include #include "absl/memory/memory.h" +#include "absl/status/statusor.h" #include "gtest/gtest.h" #include "thinkit/generic_testbed.h" +#include "thinkit/proto/generic_testbed.pb.h" namespace thinkit { @@ -34,7 +36,10 @@ class GenericTestbedInterface { virtual void SetUp() = 0; virtual void TearDown() = 0; - virtual GenericTestbed& GetGenericTestbed() = 0; + // Declares the test requirements for this test and returns a testbed that can + // support them. + virtual absl::StatusOr> + GetTestbedWithRequirements(const thinkit::TestRequirements& requirements) = 0; }; // The Thinkit `TestParams` defines test parameters to @@ -84,8 +89,9 @@ class GenericTestbedFixture : public testing::TestWithParam { // Accessor for the Generic testbed. This is only safe to be called after the // SetUp has completed. - GenericTestbed& GetGenericTestbed() { - return generic_testbed_interface_->GetGenericTestbed(); + absl::StatusOr> GetTestbedWithRequirements( + const thinkit::TestRequirements& requirements) { + return generic_testbed_interface_->GetTestbedWithRequirements(requirements); } std::string GetGnmiConfig() { return GetParam().gnmi_config; } diff --git a/thinkit/packet_generation_finalizer.h b/thinkit/packet_generation_finalizer.h index 27e571bd..7bd39797 100644 --- a/thinkit/packet_generation_finalizer.h +++ b/thinkit/packet_generation_finalizer.h @@ -25,9 +25,7 @@ class PacketGenerationFinalizer { virtual ~PacketGenerationFinalizer() = 0; }; - inline PacketGenerationFinalizer::~PacketGenerationFinalizer() {} - } // namespace thinkit #endif // GOOGLE_THINKIT_PACKET_GENERATION_FINALIZER_H_ diff --git a/thinkit/proto/generic_testbed.proto b/thinkit/proto/generic_testbed.proto index af0db6b8..81edc750 100644 --- a/thinkit/proto/generic_testbed.proto +++ b/thinkit/proto/generic_testbed.proto @@ -14,7 +14,7 @@ syntax = "proto3"; -package third_party.pins_infra.thinkit; +package thinkit; option cc_generic_services = false;