From b0500b37b231b8f93a5b1b95e23d13830e11eb07 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Fri, 10 Jul 2015 16:36:59 -0700 Subject: [PATCH] Added support for Json and valid input to conformance tests. This was enabled by the recent open-sourcing of JSON support and MessageDifferencer. MessageDifferencer allows the conformance suite to expand because it allows us to write tests for payloads that parse successfully. To verify the testee's output payload, we need to parse it back into a message and compare the message instances. Comparing output bytes vs. a golden message is *not* valid, because protobufs do not have a canonical encoding (especially in the presence of maps, which have no prescribed serialization order). We only add one small JSON test for now, but with the framework in place we now have the foundation to dramatically expand the coverage of the conformance test suite. Also added the ability for the testee to skip tests that exercise features that are unimplemented. This allows Java (which currently has no JSON support) to skip tests involving JSON. Change-Id: I697b4363da432b61ae3b638b4287c4cda1af4deb --- conformance/ConformanceJava.java | 6 +- conformance/conformance.proto | 18 +-- conformance/conformance_cpp.cc | 56 ++++++++-- conformance/conformance_test.cc | 186 +++++++++++++++++++++++++++---- conformance/conformance_test.h | 25 ++++- conformance/failure_list_cpp.txt | 24 ++-- 6 files changed, 263 insertions(+), 52 deletions(-) diff --git a/conformance/ConformanceJava.java b/conformance/ConformanceJava.java index c1a53141fc..25646b8516 100644 --- a/conformance/ConformanceJava.java +++ b/conformance/ConformanceJava.java @@ -54,7 +54,7 @@ class ConformanceJava { break; } case JSON_PAYLOAD: { - return Conformance.ConformanceResponse.newBuilder().setRuntimeError("JSON not yet supported.").build(); + return Conformance.ConformanceResponse.newBuilder().setSkipped("JSON not yet supported.").build(); } case PAYLOAD_NOT_SET: { throw new RuntimeException("Request didn't have payload."); @@ -65,7 +65,7 @@ class ConformanceJava { } } - switch (request.getRequestedOutput()) { + switch (request.getRequestedOutputFormat()) { case UNSPECIFIED: throw new RuntimeException("Unspecified output format."); @@ -73,7 +73,7 @@ class ConformanceJava { return Conformance.ConformanceResponse.newBuilder().setProtobufPayload(testMessage.toByteString()).build(); case JSON: - return Conformance.ConformanceResponse.newBuilder().setRuntimeError("JSON not yet supported.").build(); + return Conformance.ConformanceResponse.newBuilder().setSkipped("JSON not yet supported.").build(); default: { throw new RuntimeException("Unexpected request output."); diff --git a/conformance/conformance.proto b/conformance/conformance.proto index 39eafdbb7c..714cbe781e 100644 --- a/conformance/conformance.proto +++ b/conformance/conformance.proto @@ -51,6 +51,12 @@ option java_package = "com.google.protobuf.conformance"; // - running as a sub-process may be more tricky in unusual environments like // iOS apps, where fork/stdin/stdout are not available. +enum WireFormat { + UNSPECIFIED = 0; + PROTOBUF = 1; + JSON = 2; +} + // Represents a single test case's input. The testee should: // // 1. parse this proto (which should always succeed) @@ -64,14 +70,8 @@ message ConformanceRequest { string json_payload = 2; } - enum RequestedOutput { - UNSPECIFIED = 0; - PROTOBUF = 1; - JSON = 2; - } - // Which format should the testee serialize its message to? - RequestedOutput requested_output = 3; + WireFormat requested_output_format = 3; } // Represents a single test case's output. @@ -96,6 +96,10 @@ message ConformanceResponse { // If the input was successfully parsed and the requested output was JSON, // serialize to JSON and set it in this field. string json_payload = 4; + + // For when the testee skipped the test, likely because a certain feature + // wasn't supported, like JSON input/output. + string skipped = 5; } } diff --git a/conformance/conformance_cpp.cc b/conformance/conformance_cpp.cc index ff47fbbffd..863b6a5b10 100644 --- a/conformance/conformance_cpp.cc +++ b/conformance/conformance_cpp.cc @@ -33,14 +33,33 @@ #include #include "conformance.pb.h" +#include +#include -using std::string; using conformance::ConformanceRequest; using conformance::ConformanceResponse; using conformance::TestAllTypes; +using google::protobuf::Descriptor; +using google::protobuf::DescriptorPool; +using google::protobuf::internal::scoped_ptr; +using google::protobuf::util::BinaryToJsonString; +using google::protobuf::util::JsonToBinaryString; +using google::protobuf::util::NewTypeResolverForDescriptorPool; +using google::protobuf::util::Status; +using google::protobuf::util::TypeResolver; +using std::string; + +static const char kTypeUrlPrefix[] = "type.googleapis.com"; + +static string GetTypeUrl(const Descriptor* message) { + return string(kTypeUrlPrefix) + "/" + message->full_name(); +} int test_count = 0; bool verbose = false; +TypeResolver* type_resolver; +string* type_url; + bool CheckedRead(int fd, void *buf, size_t len) { size_t ofs = 0; @@ -79,27 +98,43 @@ void DoTest(const ConformanceRequest& request, ConformanceResponse* response) { } break; - case ConformanceRequest::kJsonPayload: - response->set_runtime_error("JSON input is not yet supported."); + case ConformanceRequest::kJsonPayload: { + string proto_binary; + Status status = JsonToBinaryString(type_resolver, *type_url, + request.json_payload(), &proto_binary); + if (!status.ok()) { + response->set_parse_error(string("Parse error: ") + + status.error_message().as_string()); + return; + } + + GOOGLE_CHECK(test_message.ParseFromString(proto_binary)); break; + } case ConformanceRequest::PAYLOAD_NOT_SET: GOOGLE_LOG(FATAL) << "Request didn't have payload."; break; } - switch (request.requested_output()) { - case ConformanceRequest::UNSPECIFIED: + switch (request.requested_output_format()) { + case conformance::UNSPECIFIED: GOOGLE_LOG(FATAL) << "Unspecified output format"; break; - case ConformanceRequest::PROTOBUF: - test_message.SerializeToString(response->mutable_protobuf_payload()); + case conformance::PROTOBUF: + GOOGLE_CHECK( + test_message.SerializeToString(response->mutable_protobuf_payload())); break; - case ConformanceRequest::JSON: - response->set_runtime_error("JSON output is not yet supported."); + case conformance::JSON: { + string proto_binary; + GOOGLE_CHECK(test_message.SerializeToString(&proto_binary)); + Status status = BinaryToJsonString(type_resolver, *type_url, proto_binary, + response->mutable_json_payload()); + GOOGLE_CHECK(status.ok()); break; + } } } @@ -146,6 +181,9 @@ bool DoTestIo() { } int main() { + type_resolver = NewTypeResolverForDescriptorPool( + kTypeUrlPrefix, DescriptorPool::generated_pool()); + type_url = new string(GetTypeUrl(TestAllTypes::descriptor())); while (1) { if (!DoTestIo()) { fprintf(stderr, "conformance-cpp: received EOF from test runner " diff --git a/conformance/conformance_test.cc b/conformance/conformance_test.cc index 7a7fc6f5bf..0ee201f3c5 100644 --- a/conformance/conformance_test.cc +++ b/conformance/conformance_test.cc @@ -35,18 +35,34 @@ #include "conformance_test.h" #include #include +#include +#include +#include +#include #include using conformance::ConformanceRequest; using conformance::ConformanceResponse; using conformance::TestAllTypes; +using conformance::WireFormat; using google::protobuf::Descriptor; using google::protobuf::FieldDescriptor; using google::protobuf::internal::WireFormatLite; +using google::protobuf::TextFormat; +using google::protobuf::util::JsonToBinaryString; +using google::protobuf::util::MessageDifferencer; +using google::protobuf::util::NewTypeResolverForDescriptorPool; +using google::protobuf::util::Status; using std::string; namespace { +static const char kTypeUrlPrefix[] = "type.googleapis.com"; + +static string GetTypeUrl(const Descriptor* message) { + return string(kTypeUrlPrefix) + "/" + message->full_name(); +} + /* Routines for building arbitrary protos *************************************/ // We would use CodedOutputStream except that we want more freedom to build @@ -162,9 +178,13 @@ void ConformanceTestSuite::ReportSuccess(const string& test_name) { } void ConformanceTestSuite::ReportFailure(const string& test_name, + const ConformanceRequest& request, + const ConformanceResponse& response, const char* fmt, ...) { if (expected_to_fail_.erase(test_name) == 1) { - StringAppendF(&output_, "FAILED AS EXPECTED, test=%s: ", test_name.c_str()); + expected_failures_++; + if (!verbose_) + return; } else { StringAppendF(&output_, "ERROR, test=%s: ", test_name.c_str()); unexpected_failing_tests_.insert(test_name); @@ -173,7 +193,20 @@ void ConformanceTestSuite::ReportFailure(const string& test_name, va_start(args, fmt); StringAppendV(&output_, fmt, args); va_end(args); - failures_++; + StringAppendF(&output_, " request=%s, response=%s\n", + request.ShortDebugString().c_str(), + response.ShortDebugString().c_str()); +} + +void ConformanceTestSuite::ReportSkip(const string& test_name, + const ConformanceRequest& request, + const ConformanceResponse& response) { + if (verbose_) { + StringAppendF(&output_, "SKIPPED, test=%s request=%s, response=%s\n", + test_name.c_str(), request.ShortDebugString().c_str(), + response.ShortDebugString().c_str()); + } + skipped_.insert(test_name); } void ConformanceTestSuite::RunTest(const string& test_name, @@ -202,26 +235,117 @@ void ConformanceTestSuite::RunTest(const string& test_name, } } +void ConformanceTestSuite::RunValidInputTest( + const string& test_name, const string& input, WireFormat input_format, + const string& equivalent_text_format, WireFormat requested_output) { + TestAllTypes reference_message; + GOOGLE_CHECK( + TextFormat::ParseFromString(equivalent_text_format, &reference_message)); + + ConformanceRequest request; + ConformanceResponse response; + + switch (input_format) { + case conformance::PROTOBUF: + request.set_protobuf_payload(input); + break; + + case conformance::JSON: + request.set_json_payload(input); + break; + + case conformance::UNSPECIFIED: + GOOGLE_LOG(FATAL) << "Unspecified input format"; + + } + + request.set_requested_output_format(requested_output); + + RunTest(test_name, request, &response); + + TestAllTypes test_message; + + switch (response.result_case()) { + case ConformanceResponse::kParseError: + case ConformanceResponse::kRuntimeError: + ReportFailure(test_name, request, response, + "Failed to parse valid JSON input."); + return; + + case ConformanceResponse::kSkipped: + ReportSkip(test_name, request, response); + return; + + case ConformanceResponse::kJsonPayload: { + if (requested_output != conformance::JSON) { + ReportFailure( + test_name, request, response, + "Test was asked for protobuf output but provided JSON instead."); + return; + } + string binary_protobuf; + Status status = + JsonToBinaryString(type_resolver_.get(), type_url_, + response.json_payload(), &binary_protobuf); + if (!status.ok()) { + ReportFailure(test_name, request, response, + "JSON output we received from test was unparseable."); + return; + } + + GOOGLE_CHECK(test_message.ParseFromString(binary_protobuf)); + break; + } + + case ConformanceResponse::kProtobufPayload: { + if (requested_output != conformance::PROTOBUF) { + ReportFailure( + test_name, request, response, + "Test was asked for JSON output but provided protobuf instead."); + return; + } + + if (!test_message.ParseFromString(response.protobuf_payload())) { + ReportFailure(test_name, request, response, + "Protobuf output we received from test was unparseable."); + return; + } + + break; + } + } + + MessageDifferencer differencer; + string differences; + differencer.ReportDifferencesToString(&differences); + + if (differencer.Equals(reference_message, test_message)) { + ReportSuccess(test_name); + } else { + ReportFailure(test_name, request, response, + "Output was not equivalent to reference message: %s.", + differences.c_str()); + } +} + // Expect that this precise protobuf will cause a parse error. void ConformanceTestSuite::ExpectParseFailureForProto( const string& proto, const string& test_name) { ConformanceRequest request; ConformanceResponse response; request.set_protobuf_payload(proto); + string effective_test_name = "ProtobufInput." + test_name; // We don't expect output, but if the program erroneously accepts the protobuf // we let it send its response as this. We must not leave it unspecified. - request.set_requested_output(ConformanceRequest::PROTOBUF); + request.set_requested_output_format(conformance::PROTOBUF); - RunTest(test_name, request, &response); + RunTest(effective_test_name, request, &response); if (response.result_case() == ConformanceResponse::kParseError) { - ReportSuccess(test_name); + ReportSuccess(effective_test_name); } else { - ReportFailure(test_name, - "Should have failed to parse, but didn't. Request: %s, " - "response: %s\n", - request.ShortDebugString().c_str(), - response.ShortDebugString().c_str()); + ReportFailure(effective_test_name, request, response, + "Should have failed to parse, but didn't."); } } @@ -235,6 +359,16 @@ void ConformanceTestSuite::ExpectHardParseFailureForProto( return ExpectParseFailureForProto(proto, test_name); } +void ConformanceTestSuite::RunValidJsonTest( + const string& test_name, const string& input_json, + const string& equivalent_text_format) { + RunValidInputTest("JsonInput." + test_name + ".JsonOutput", input_json, + conformance::JSON, equivalent_text_format, + conformance::PROTOBUF); + RunValidInputTest("JsonInput." + test_name + ".ProtobufOutput", input_json, conformance::JSON, + equivalent_text_format, conformance::JSON); +} + void ConformanceTestSuite::TestPrematureEOFForType(FieldDescriptor::Type type) { // Incomplete values for each wire type. static const string incompletes[6] = { @@ -333,11 +467,12 @@ bool ConformanceTestSuite::CheckSetEmpty(const set& set_to_check, return true; } else { StringAppendF(&output_, "\n"); - StringAppendF(&output_, "ERROR: %s:\n", msg); + StringAppendF(&output_, "%s:\n", msg); for (set::const_iterator iter = set_to_check.begin(); iter != set_to_check.end(); ++iter) { - StringAppendF(&output_, "%s\n", iter->c_str()); + StringAppendF(&output_, " %s\n", iter->c_str()); } + StringAppendF(&output_, "\n"); return false; } } @@ -345,23 +480,25 @@ bool ConformanceTestSuite::CheckSetEmpty(const set& set_to_check, bool ConformanceTestSuite::RunSuite(ConformanceTestRunner* runner, std::string* output) { runner_ = runner; - output_.clear(); successes_ = 0; - failures_ = 0; + expected_failures_ = 0; + skipped_.clear(); test_names_.clear(); unexpected_failing_tests_.clear(); unexpected_succeeding_tests_.clear(); + type_resolver_.reset(NewTypeResolverForDescriptorPool( + kTypeUrlPrefix, DescriptorPool::generated_pool())); + type_url_ = GetTypeUrl(TestAllTypes::descriptor()); + + output_ = "\nCONFORMANCE TEST BEGIN ====================================\n\n"; for (int i = 1; i <= FieldDescriptor::MAX_TYPE; i++) { if (i == FieldDescriptor::TYPE_GROUP) continue; TestPrematureEOFForType(static_cast(i)); } - StringAppendF(&output_, "\n"); - StringAppendF(&output_, - "CONFORMANCE SUITE FINISHED: completed %d tests, %d successes, " - "%d failures.\n", - successes_ + failures_, successes_, failures_); + RunValidJsonTest("HelloWorld", "{\"optionalString\":\"Hello, World!\"}", + "optional_string: 'Hello, World!'"); bool ok = CheckSetEmpty(expected_to_fail_, @@ -377,6 +514,17 @@ bool ConformanceTestSuite::RunSuite(ConformanceTestRunner* runner, "These tests succeeded, even though they were listed in " "the failure list. Remove them from the failure list"); + CheckSetEmpty(skipped_, + "These tests were skipped (probably because support for some " + "features is not implemented)"); + + StringAppendF(&output_, + "CONFORMANCE SUITE %s: %d successes, %d skipped, " + "%d expected failures, %d unexpected failures.\n", + ok ? "PASSED" : "FAILED", successes_, skipped_.size(), + expected_failures_, unexpected_failing_tests_.size()); + StringAppendF(&output_, "\n"); + output->assign(output_); return ok; diff --git a/conformance/conformance_test.h b/conformance/conformance_test.h index 764a8d336d..cadda82881 100644 --- a/conformance/conformance_test.h +++ b/conformance/conformance_test.h @@ -39,6 +39,8 @@ #define CONFORMANCE_CONFORMANCE_TEST_H #include +#include +#include #include namespace conformance { @@ -98,10 +100,22 @@ class ConformanceTestSuite { private: void ReportSuccess(const std::string& test_name); - void ReportFailure(const std::string& test_name, const char* fmt, ...); + void ReportFailure(const string& test_name, + const conformance::ConformanceRequest& request, + const conformance::ConformanceResponse& response, + const char* fmt, ...); + void ReportSkip(const string& test_name, + const conformance::ConformanceRequest& request, + const conformance::ConformanceResponse& response); void RunTest(const std::string& test_name, const conformance::ConformanceRequest& request, conformance::ConformanceResponse* response); + void RunValidInputTest(const string& test_name, const string& input, + conformance::WireFormat input_format, + const string& equivalent_text_format, + conformance::WireFormat requested_output); + void RunValidJsonTest(const string& test_name, const string& input_json, + const string& equivalent_text_format); void ExpectParseFailureForProto(const std::string& proto, const std::string& test_name); void ExpectHardParseFailureForProto(const std::string& proto, @@ -110,7 +124,7 @@ class ConformanceTestSuite { bool CheckSetEmpty(const set& set_to_check, const char* msg); ConformanceTestRunner* runner_; int successes_; - int failures_; + int expected_failures_; bool verbose_; std::string output_; @@ -127,6 +141,13 @@ class ConformanceTestSuite { // The set of tests that succeeded, but weren't expected to. std::set unexpected_succeeding_tests_; + + // The set of tests that the testee opted out of; + std::set skipped_; + + google::protobuf::internal::scoped_ptr + type_resolver_; + std::string type_url_; }; } // namespace protobuf diff --git a/conformance/failure_list_cpp.txt b/conformance/failure_list_cpp.txt index d792eddd62..53d90c9928 100644 --- a/conformance/failure_list_cpp.txt +++ b/conformance/failure_list_cpp.txt @@ -7,15 +7,15 @@ # TODO(haberman): insert links to corresponding bugs tracking the issue. # Should we use GitHub issues or the Google-internal bug tracker? -PrematureEofBeforeKnownRepeatedValue.MESSAGE -PrematureEofInDelimitedDataForKnownNonRepeatedValue.MESSAGE -PrematureEofInDelimitedDataForKnownRepeatedValue.MESSAGE -PrematureEofInPackedField.BOOL -PrematureEofInPackedField.ENUM -PrematureEofInPackedField.INT32 -PrematureEofInPackedField.INT64 -PrematureEofInPackedField.SINT32 -PrematureEofInPackedField.SINT64 -PrematureEofInPackedField.UINT32 -PrematureEofInPackedField.UINT64 -PrematureEofInsideKnownRepeatedValue.MESSAGE +ProtobufInput.PrematureEofBeforeKnownRepeatedValue.MESSAGE +ProtobufInput.PrematureEofInDelimitedDataForKnownNonRepeatedValue.MESSAGE +ProtobufInput.PrematureEofInDelimitedDataForKnownRepeatedValue.MESSAGE +ProtobufInput.PrematureEofInPackedField.BOOL +ProtobufInput.PrematureEofInPackedField.ENUM +ProtobufInput.PrematureEofInPackedField.INT32 +ProtobufInput.PrematureEofInPackedField.INT64 +ProtobufInput.PrematureEofInPackedField.SINT32 +ProtobufInput.PrematureEofInPackedField.SINT64 +ProtobufInput.PrematureEofInPackedField.UINT32 +ProtobufInput.PrematureEofInPackedField.UINT64 +ProtobufInput.PrematureEofInsideKnownRepeatedValue.MESSAGE