Skip to content

Commit

Permalink
[Dvaas] Compare ports of dvaas::Packet during comparison in CompareSw…
Browse files Browse the repository at this point in the history
…itchOutputs. (#753)



Co-authored-by: kheradmandG <[email protected]>
Co-authored-by: kishanps <[email protected]>
  • Loading branch information
3 people authored Nov 25, 2024
1 parent d62a47c commit b41d1bc
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 35 deletions.
65 changes: 40 additions & 25 deletions dvaas/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
],
)

Expand All @@ -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",
],
)
19 changes: 14 additions & 5 deletions dvaas/test_run_validation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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(), "'");
Expand Down
12 changes: 7 additions & 5 deletions dvaas/test_run_validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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
Expand All @@ -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_
136 changes: 136 additions & 0 deletions dvaas/test_run_validation_test.cc
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit b41d1bc

Please sign in to comment.