[Thinkit] Replaced GetGenericTestbed with GetTestbedWithRequirements …

…to take in TestbedRequiredments proto. (sonic-net#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 <[email protected]>
Co-authored-by: bhagatgit <[email protected]>

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",

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",
     ],
 )

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<std::string> 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);
 }
 

thinkit/bazel_test_environment.h

@@ -18,30 +18,48 @@
 #include <ios>
 
 #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<void(absl::string_view)> 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<void(absl::string_view)> set_test_case_id_;
+  // The mutex is used to ensure that writes to disk are sequential.
+  absl::Mutex write_mutex_;
 };
 
 }  // namespace thinkit

thinkit/bazel_test_environment_test.cc

@@ -1,32 +1,130 @@
 #include "thinkit/bazel_test_environment.h"
 
+#include <cstdlib>
+#include <memory>
+#include <string>
+
+// 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<TestEnvironment> environment_ =
+TEST(BazelTestEnvironmentTest, StoreTestArtifact) {
+  std::unique_ptr<TestEnvironment> environment =
       absl::make_unique<BazelTestEnvironment>(/*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<TestEnvironment> environment =
+      absl::make_unique<BazelTestEnvironment>(/*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<BazelTestEnvironment> environment =
+      absl::make_unique<BazelTestEnvironment>(/*mask_known_failures=*/true);
+  auto foo_proto = gutil::ParseProtoOrDie<google::protobuf::Int32Value>(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<TestEnvironment> environment =
+      absl::make_unique<BazelTestEnvironment>(
+          /*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<TestEnvironment> environment =
+      absl::make_unique<BazelTestEnvironment>(/*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<int64_t>(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

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<HttpResponse> SendRestRequestToIxia(
       RequestType type, absl::string_view url, absl::string_view payload) = 0;
 };

thinkit/generic_testbed_fixture.h

@@ -18,8 +18,10 @@
 #include <memory>
 
 #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<std::unique_ptr<GenericTestbed>>
+  GetTestbedWithRequirements(const thinkit::TestRequirements& requirements) = 0;
 };
 
 // The Thinkit `TestParams` defines test parameters to
@@ -84,8 +89,9 @@ class GenericTestbedFixture : public testing::TestWithParam<TestParams> {
 
   // 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<std::unique_ptr<GenericTestbed>> GetTestbedWithRequirements(
+      const thinkit::TestRequirements& requirements) {
+    return generic_testbed_interface_->GetTestbedWithRequirements(requirements);
   }
 
   std::string GetGnmiConfig() { return GetParam().gnmi_config; }

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_

thinkit/proto/generic_testbed.proto

@@ -14,7 +14,7 @@
 
 syntax = "proto3";
 
-package third_party.pins_infra.thinkit;
+package thinkit;
 
 option cc_generic_services = false;