From b41d1bcd6e308ef048ce0ab2c4803d788b987320 Mon Sep 17 00:00:00 2001 From: VSuryaprasad-hcl <159443973+VSuryaprasad-HCL@users.noreply.github.com> Date: Mon, 25 Nov 2024 19:32:51 +0000 Subject: [PATCH] [Dvaas] Compare ports of dvaas::Packet during comparison in CompareSwitchOutputs. (#753) Co-authored-by: kheradmandG Co-authored-by: kishanps --- dvaas/BUILD.bazel | 65 ++++++++------ dvaas/test_run_validation.cc | 19 +++-- dvaas/test_run_validation.h | 12 +-- dvaas/test_run_validation_test.cc | 136 ++++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 35 deletions(-) create mode 100644 dvaas/test_run_validation_test.cc diff --git a/dvaas/BUILD.bazel b/dvaas/BUILD.bazel index 1852460b..a7179a9a 100644 --- a/dvaas/BUILD.bazel +++ b/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", + ], +) \ No newline at end of file diff --git a/dvaas/test_run_validation.cc b/dvaas/test_run_validation.cc index 32d147b3..55791394 100644 --- a/dvaas/test_run_validation.cc +++ b/dvaas/test_run_validation.cc @@ -121,7 +121,8 @@ 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())) { @@ -129,6 +130,13 @@ bool CompareSwitchOutputs(SwitchOutput actual_output, << 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) { @@ -136,7 +144,8 @@ bool CompareSwitchOutputs(SwitchOutput actual_output, 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(), "'"); diff --git a/dvaas/test_run_validation.h b/dvaas/test_run_validation.h index 49cb1163..1d78f43d 100644 --- a/dvaas/test_run_validation.h +++ b/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 #include @@ -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 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 + 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_ diff --git a/dvaas/test_run_validation_test.cc b/dvaas/test_run_validation_test.cc new file mode 100644 index 00000000..0a166bf0 --- /dev/null +++ b/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(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(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(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(R"pb( + test_vector { acceptable_outputs { packets { port: "1" } } } + actual_output { packets { port: "2" } } + )pb")) + .failure() + .description(), + HasSubstr("mismatched ports:")); +} + +} // namespace +} // namespace dvaas