[Dvaas] Compare ports of dvaas::Packet during comparison in CompareSwitchOutputs.

dvaas/BUILD.bazel

@@ -165,31 +165,6 @@ cc_library(
     ],
 )
 
-# go/golden-test-with-coverage
-cc_test(
-    name = "test_vector_stats_test",
-    srcs = ["test_vector_stats_test.cc"],
-    linkstatic = True,
-    deps = [
-        ":test_vector_cc_proto",
-        ":test_vector_stats",
-        "//gutil:testing",
-        "@com_google_googletest//:gtest_main",
-    ],
-)
-
-cmd_diff_test(
-    name = "test_vector_stats_diff_test",
-    actual_cmd = " | ".join([
-        "$(execpath :test_vector_stats_test)",
-        # Strip unnecessary lines for golden testing.
-        "sed '1,/^\\[ RUN/d'",  # Strip everything up to a line beginning with '[ RUN'.
-        "sed '/^\\[/d'",  # Strip every line beginning with '['.
-    ]),
-    expected = "test_vector_stats_test.expected",
-    tools = [":test_vector_stats_test"],
-)
-
 cc_library(
     name = "mirror_testbed_config",
     testonly = True,
@@ -293,6 +268,7 @@ cc_test(
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_googletest//:gtest_main",
+        "@com_google_protobuf//:protobuf",
     ],
 )
 
@@ -307,3 +283,42 @@ cmd_diff_test(
     expected = "user_provided_packet_test_vector_test.expected",
     tools = [":user_provided_packet_test_vector_test"],
 )
+
+# go/golden-test-with-coverage
+cc_test(
+    name = "test_vector_stats_test",
+    srcs = ["test_vector_stats_test.cc"],
+    linkstatic = True,
+    deps = [
+        ":test_vector_cc_proto",
+        ":test_vector_stats",
+        "//gutil:testing",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
+cmd_diff_test(
+    name = "test_vector_stats_diff_test",
+    actual_cmd = " | ".join([
+        "$(execpath :test_vector_stats_test)",
+        # Strip unnecessary lines for golden testing.
+        "sed '1,/^\\[ RUN/d'",  # Strip everything up to a line beginning with '[ RUN'.
+        "sed '/^\\[/d'",  # Strip every line beginning with '['.
+    ]),
+    expected = "test_vector_stats_test.expected",
+    tools = [":test_vector_stats_test"],
+)
+
+cc_test(
+    name = "test_run_validation_test",
+    srcs = ["test_run_validation_test.cc"],
+    deps = [
+        ":test_run_validation",
+        ":test_vector_cc_proto",
+        "//gutil:proto_matchers",
+        "//gutil:testing",
+        "//p4_pdpi/packetlib:packetlib_cc_proto",
+        "@com_google_googletest//:gtest_main",
+        "@com_google_protobuf//:protobuf",
+    ],
+)

dvaas/test_run_validation.cc

@@ -121,22 +121,31 @@ bool CompareSwitchOutputs(SwitchOutput actual_output,
     const Packet& actual_packet = actual_output.packets(i);
     const Packet& expected_packet = expected_output.packets(i);
     MessageDifferencer differ;
-    for (auto* field : params.ignored_fields) differ.IgnoreField(field);
+    for (auto* field : params.ignored_packetlib_fields)
+      differ.IgnoreField(field);
     std::string diff;
     differ.ReportDifferencesToString(&diff);
     if (!differ.Compare(expected_packet.parsed(), actual_packet.parsed())) {
       *listener << "has packet " << i << " with mismatched header fields:\n  "
                 << Indent(2, diff);
       return false;
     }
+    if (expected_packet.port() != actual_packet.port()) {
+      *listener << "has packet " << i << " with mismatched ports:\n"
+                << absl::Substitute("  \"$0\" -> \"$1\"",
+                                    expected_packet.port(),
+                                    actual_packet.port());
+      return false;
+    }
   }
 
   for (int i = 0; i < expected_output.packet_ins_size(); ++i) {
     const PacketIn& actual_packet_in = actual_output.packet_ins(i);
     const PacketIn& expected_packet_in = expected_output.packet_ins(i);
 
     MessageDifferencer differ;
-    for (auto* field : params.ignored_fields) differ.IgnoreField(field);
+    for (auto* field : params.ignored_packetlib_fields)
+      differ.IgnoreField(field);
     std::string diff;
     differ.ReportDifferencesToString(&diff);
     if (!differ.Compare(expected_packet_in.parsed(),
@@ -164,7 +173,7 @@ bool CompareSwitchOutputs(SwitchOutput actual_output,
                           actual_metadata->value())) {
         *listener << "has packet in " << i
                   << " with mismatched value for metadata field '"
-                  << expected_metadata.name() << "':\n " << Indent(2, diff);
+                  << expected_metadata.name() << "':\n  " << Indent(2, diff);
         return false;
       }
     }
@@ -351,10 +360,10 @@ PacketTestValidationResult ValidateTestRun(
 
   std::string expectation = DescribeExpectation(
       test_run.test_vector().input(), acceptable_output_characterizations);
-  if (!diff_params.ignored_fields.empty()) {
+  if (!diff_params.ignored_packetlib_fields.empty()) {
     absl::StrAppend(
         &expectation, "\n          (ignoring the field(s) ",
-        absl::StrJoin(diff_params.ignored_fields, ",",
+        absl::StrJoin(diff_params.ignored_packetlib_fields, ",",
                       [](std::string* out,
                          const google::protobuf::FieldDescriptor* field) {
                         absl::StrAppend(out, "'", field->name(), "'");

dvaas/test_run_validation.h

@@ -12,8 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef PINS_DVAAS_test_run_validation_H_
-#define PINS_DVAAS_test_run_validation_H_
+#ifndef PINS_test_run_validation_H_
+#define PINS_test_run_validation_H_
 
 #include <string>
 #include <vector>
@@ -32,8 +32,10 @@ namespace dvaas {
 struct SwitchOutputDiffParams {
   // Used to skip packet fields where model and switch are known to have
   // different behavior, which we don't want to test. All FieldDescriptors
-  // should belong to packetlib::Packet.
-  std::vector<const google::protobuf::FieldDescriptor*> ignored_fields;
+  // should belong to packetlib::Packet. The ignored fields are ignored both in
+  // normal (i.e. front-panel) packets and in packet-ins.
+  std::vector<const google::protobuf::FieldDescriptor*>
+      ignored_packetlib_fields;
 
   // Used to skip packet-in metadata where model and switch are known to have
   // different behavior, which we don't want to test. If a packet-in metadata
@@ -57,4 +59,4 @@ absl::Status ValidateTestRuns(const PacketTestRuns& test_runs,
 
 }  // namespace dvaas
 
-#endif  // PINS_DVAAS_test_run_validation_H_
+#endif  // PINS_test_run_validation_H_

dvaas/test_run_validation_test.cc

@@ -0,0 +1,136 @@
+#include "dvaas/test_run_validation.h"
+
+#include "dvaas/test_vector.pb.h"
+#include "gmock/gmock.h"
+#include "google/protobuf/descriptor.h"
+#include "gtest/gtest.h"
+#include "gutil/proto_matchers.h"
+#include "gutil/testing.h"
+#include "p4_pdpi/packetlib/packetlib.pb.h"
+
+namespace dvaas {
+namespace {
+
+using gutil::EqualsProto;
+using testing::HasSubstr;
+
+TEST(TestRunValidationTest,
+     PacketFieldIsIgnoredIfAndOnlyIfIncludedInIgnoredPacketlibFields) {
+  const PacketTestRun test_run = gutil::ParseProtoOrDie<PacketTestRun>(R"pb(
+    test_vector {
+      acceptable_outputs {
+        packets {
+          port: "1"
+          parsed { headers { ethernet_header { ethertype: "0x0888" } } }
+        }
+      }
+    }
+    actual_output {
+      packets {
+        port: "1"
+        parsed { headers { ethernet_header { ethertype: "0x0111" } } }
+      }
+    }
+  )pb");
+
+  // Without ignoring ethertype, validation must fail.
+  ASSERT_THAT(ValidateTestRun(test_run).failure().description(),
+              HasSubstr("modified:"));
+
+  const google::protobuf::FieldDescriptor* ethertype_field_descriptor =
+      packetlib::EthernetHeader::descriptor()->FindFieldByName("ethertype");
+  ASSERT_THAT(ethertype_field_descriptor, testing::NotNull());
+
+  // Ignoring ethertype, validation must succeed.
+  ASSERT_THAT(ValidateTestRun(
+                  test_run,
+                  {
+                      .ignored_packetlib_fields = {ethertype_field_descriptor},
+                  }),
+              EqualsProto(R"pb()pb"));
+}
+
+TEST(TestRunValidationTest,
+     PacketInFieldIsIgnoredIfAndOnlyIfIncludedInIgnoredPacketlibFields) {
+  const PacketTestRun test_run = gutil::ParseProtoOrDie<PacketTestRun>(R"pb(
+    test_vector {
+      acceptable_outputs {
+        packet_ins {
+          parsed { headers { ethernet_header { ethertype: "0x0888" } } }
+        }
+      }
+    }
+    actual_output {
+      packet_ins {
+        parsed { headers { ethernet_header { ethertype: "0x0111" } } }
+      }
+    }
+  )pb");
+
+  // Without ignoring ethertype, validation must fail.
+  ASSERT_THAT(ValidateTestRun(test_run).failure().description(),
+              HasSubstr("modified:"));
+
+  const google::protobuf::FieldDescriptor* ethertype_field_descriptor =
+      packetlib::EthernetHeader::descriptor()->FindFieldByName("ethertype");
+  ASSERT_THAT(ethertype_field_descriptor, testing::NotNull());
+
+  // Ignoring ethertype, validation must succeed.
+  ASSERT_THAT(ValidateTestRun(
+                  test_run,
+                  {
+                      .ignored_packetlib_fields = {ethertype_field_descriptor},
+                  }),
+              EqualsProto(R"pb()pb"));
+}
+
+TEST(TestRunValidationTest,
+     PacketInMetadataIsIgnoredIfAndOnlyIfIncludedInIgnoredPacketinMetadata) {
+  const PacketTestRun test_run = gutil::ParseProtoOrDie<PacketTestRun>(R"pb(
+    test_vector {
+      acceptable_outputs {
+        packet_ins {
+          metadata {
+            name: "target_egress_port"
+            value { str: "1" }
+          }
+        }
+      }
+    }
+    actual_output {
+      packet_ins {
+        metadata {
+          name: "target_egress_port"
+          value { str: "2" }
+        }
+      }
+    }
+  )pb");
+
+  // Without ignoring target_egress_port, validation must fail.
+  ASSERT_THAT(ValidateTestRun(test_run).failure().description(),
+              HasSubstr("modified:"));
+
+  // Ignoring target_egress_port, validation must succeed.
+  ASSERT_THAT(
+      ValidateTestRun(test_run,
+                      {
+                          .ignored_packet_in_metadata = {"target_egress_port"},
+                      }),
+      EqualsProto(R"pb()pb"));
+}
+
+TEST(TestRunValidationTest,
+     DifferenceBetweenPortsInActualAndAcceptableOutputLeadToFailure) {
+  // Without ignoring target_egress_port, validation must fail.
+  ASSERT_THAT(ValidateTestRun(gutil::ParseProtoOrDie<PacketTestRun>(R"pb(
+                test_vector { acceptable_outputs { packets { port: "1" } } }
+                actual_output { packets { port: "2" } }
+              )pb"))
+                  .failure()
+                  .description(),
+              HasSubstr("mismatched ports:"));
+}
+
+}  // namespace
+}  // namespace dvaas