From ef7894e2dc6d287419e42a4fdc52cdfedd386d16 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 16 May 2016 12:45:02 -0700 Subject: [PATCH] Make conformance tests more strict about the failure list. The failure lists were falling out of date because the tests would pass even if people forgot to remove failures from the list. --- conformance/conformance_test.cc | 55 +++++++++++++++++------- conformance/conformance_test.h | 10 ++++- conformance/conformance_test_runner.cc | 4 +- conformance/failure_list_cpp.txt | 58 -------------------------- conformance/failure_list_java.txt | 4 -- 5 files changed, 51 insertions(+), 80 deletions(-) diff --git a/conformance/conformance_test.cc b/conformance/conformance_test.cc index fc0605bf..59a61e51 100644 --- a/conformance/conformance_test.cc +++ b/conformance/conformance_test.cc @@ -30,6 +30,7 @@ #include #include +#include #include "conformance.pb.h" #include "conformance_test.h" @@ -575,24 +576,41 @@ void ConformanceTestSuite::TestPrematureEOFForType(FieldDescriptor::Type type) { } } -void ConformanceTestSuite::SetFailureList(const vector& failure_list) { +void ConformanceTestSuite::SetFailureList(const string& filename, + const vector& failure_list) { + failure_list_filename_ = filename; expected_to_fail_.clear(); std::copy(failure_list.begin(), failure_list.end(), std::inserter(expected_to_fail_, expected_to_fail_.end())); } bool ConformanceTestSuite::CheckSetEmpty(const set& set_to_check, - const char* msg) { + const std::string& write_to_file, + const std::string& msg) { if (set_to_check.empty()) { return true; } else { StringAppendF(&output_, "\n"); - StringAppendF(&output_, "%s:\n", msg); + StringAppendF(&output_, "%s\n\n", msg.c_str()); for (set::const_iterator iter = set_to_check.begin(); iter != set_to_check.end(); ++iter) { StringAppendF(&output_, " %s\n", iter->c_str()); } StringAppendF(&output_, "\n"); + + if (!write_to_file.empty()) { + std::ofstream os(write_to_file); + if (os) { + for (set::const_iterator iter = set_to_check.begin(); + iter != set_to_check.end(); ++iter) { + os << *iter << "\n"; + } + } else { + StringAppendF(&output_, "Failed to open file: %s\n", + write_to_file.c_str()); + } + } + return false; } } @@ -1965,27 +1983,34 @@ bool ConformanceTestSuite::RunSuite(ConformanceTestRunner* runner, )"); bool ok = true; - if (!CheckSetEmpty(expected_to_fail_, + if (!CheckSetEmpty(expected_to_fail_, "nonexistent_tests.txt", "These tests were listed in the failure list, but they " - "don't exist. Remove them from the failure list")) { + "don't exist. Remove them from the failure list by " + "running:\n" + " ./update_failure_list.py " + failure_list_filename_ + + " --remove nonexistent_tests.txt")) { ok = false; } - if (!CheckSetEmpty(unexpected_failing_tests_, + if (!CheckSetEmpty(unexpected_failing_tests_, "failing_tests.txt", "These tests failed. If they can't be fixed right now, " "you can add them to the failure list so the overall " - "suite can succeed")) { + "suite can succeed. Add them to the failure list by " + "running:\n" + " ./update_failure_list.py " + failure_list_filename_ + + " --add failing_tests.txt")) { + ok = false; + } + if (!CheckSetEmpty(unexpected_succeeding_tests_, "succeeding_tests.txt", + "These tests succeeded, even though they were listed in " + "the failure list. Remove them from the failure list " + "by running:\n" + " ./update_failure_list.py " + failure_list_filename_ + + " --remove succeeding_tests.txt")) { ok = false; } - // Sometimes the testee may be fixed before we update the failure list (e.g., - // the testee is from a different component). We warn about this case but - // don't consider it an overall test failure. - CheckSetEmpty(unexpected_succeeding_tests_, - "These tests succeeded, even though they were listed in " - "the failure list. Remove them from the failure list"); - if (verbose_) { - CheckSetEmpty(skipped_, + CheckSetEmpty(skipped_, "", "These tests were skipped (probably because support for some " "features is not implemented)"); } diff --git a/conformance/conformance_test.h b/conformance/conformance_test.h index 75fc97bc..c9c5213c 100644 --- a/conformance/conformance_test.h +++ b/conformance/conformance_test.h @@ -98,7 +98,11 @@ class ConformanceTestSuite { // Sets the list of tests that are expected to fail when RunSuite() is called. // RunSuite() will fail unless the set of failing tests is exactly the same // as this list. - void SetFailureList(const std::vector& failure_list); + // + // The filename here is *only* used to create/format useful error messages for + // how to update the failure list. We do NOT read this file at all. + void SetFailureList(const std::string& filename, + const std::vector& failure_list); // Run all the conformance tests against the given test runner. // Test output will be stored in "output". @@ -143,12 +147,14 @@ class ConformanceTestSuite { void ExpectHardParseFailureForProto(const std::string& proto, const std::string& test_name); void TestPrematureEOFForType(google::protobuf::FieldDescriptor::Type type); - bool CheckSetEmpty(const set& set_to_check, const char* msg); + bool CheckSetEmpty(const set& set_to_check, + const std::string& write_to_file, const std::string& msg); ConformanceTestRunner* runner_; int successes_; int expected_failures_; bool verbose_; std::string output_; + std::string failure_list_filename_; // The set of test names that are expected to fail in this run, but haven't // failed yet. diff --git a/conformance/conformance_test_runner.cc b/conformance/conformance_test_runner.cc index 376a60b9..d6b1175c 100644 --- a/conformance/conformance_test_runner.cc +++ b/conformance/conformance_test_runner.cc @@ -280,11 +280,13 @@ int main(int argc, char *argv[]) { char *program; google::protobuf::ConformanceTestSuite suite; + string failure_list_filename; vector failure_list; for (int arg = 1; arg < argc; ++arg) { if (strcmp(argv[arg], "--failure_list") == 0) { if (++arg == argc) UsageError(); + failure_list_filename = argv[arg]; ParseFailureList(argv[arg], &failure_list); } else if (strcmp(argv[arg], "--verbose") == 0) { suite.SetVerbose(true); @@ -300,7 +302,7 @@ int main(int argc, char *argv[]) { } } - suite.SetFailureList(failure_list); + suite.SetFailureList(failure_list_filename, failure_list); ForkPipeRunner runner(program); std::string output; diff --git a/conformance/failure_list_cpp.txt b/conformance/failure_list_cpp.txt index 2ddf831c..839e5210 100644 --- a/conformance/failure_list_cpp.txt +++ b/conformance/failure_list_cpp.txt @@ -12,21 +12,11 @@ FieldMaskPathsDontRoundTrip.JsonOutput FieldMaskTooManyUnderscore.JsonOutput JsonInput.AnyUnorderedTypeTag.JsonOutput JsonInput.AnyUnorderedTypeTag.ProtobufOutput -JsonInput.AnyWithValueForInteger.JsonOutput -JsonInput.AnyWithValueForInteger.ProtobufOutput -JsonInput.AnyWithValueForJsonObject.JsonOutput -JsonInput.AnyWithValueForJsonObject.ProtobufOutput JsonInput.BoolFieldDoubleQuotedFalse JsonInput.BoolFieldDoubleQuotedTrue -JsonInput.BoolFieldIntegerOne -JsonInput.BoolFieldIntegerZero -JsonInput.BytesFieldInvalidBase64Characters JsonInput.BytesFieldNoPadding JsonInput.DoubleFieldTooSmall JsonInput.DurationHasZeroFractionalDigit.Validator -JsonInput.DurationJsonInputTooLarge -JsonInput.DurationJsonInputTooSmall -JsonInput.DurationMissingS JsonInput.EnumFieldUnknownValue.Validator JsonInput.FieldMaskInvalidCharacter JsonInput.FieldNameDuplicate @@ -36,58 +26,12 @@ JsonInput.FieldNameInLowerCamelCase.Validator JsonInput.FieldNameInSnakeCase.JsonOutput JsonInput.FieldNameInSnakeCase.ProtobufOutput JsonInput.FieldNameNotQuoted -JsonInput.FloatFieldTooLarge -JsonInput.FloatFieldTooSmall -JsonInput.Int32FieldLeadingSpace -JsonInput.Int32FieldLeadingZero -JsonInput.Int32FieldMinFloatValue.JsonOutput -JsonInput.Int32FieldMinFloatValue.ProtobufOutput -JsonInput.Int32FieldMinValue.JsonOutput -JsonInput.Int32FieldMinValue.ProtobufOutput -JsonInput.Int32FieldNegativeWithLeadingZero -JsonInput.Int32FieldNotInteger -JsonInput.Int32FieldNotNumber -JsonInput.Int32FieldTooLarge -JsonInput.Int32FieldTooSmall -JsonInput.Int32FieldTrailingSpace -JsonInput.Int64FieldNotInteger -JsonInput.Int64FieldNotNumber -JsonInput.Int64FieldTooLarge -JsonInput.Int64FieldTooSmall JsonInput.MapFieldValueIsNull -JsonInput.OneofFieldDuplicate JsonInput.RepeatedFieldMessageElementIsNull JsonInput.RepeatedFieldPrimitiveElementIsNull JsonInput.RepeatedFieldTrailingComma -JsonInput.RepeatedFieldWrongElementTypeExpectingIntegersGotBool -JsonInput.RepeatedFieldWrongElementTypeExpectingIntegersGotMessage -JsonInput.RepeatedFieldWrongElementTypeExpectingIntegersGotString -JsonInput.RepeatedFieldWrongElementTypeExpectingMessagesGotBool -JsonInput.RepeatedFieldWrongElementTypeExpectingMessagesGotInt -JsonInput.RepeatedFieldWrongElementTypeExpectingMessagesGotString -JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotBool -JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotInt -JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotMessage -JsonInput.StringFieldNotAString -JsonInput.StringFieldSurrogateInWrongOrder -JsonInput.StringFieldSurrogatePair.JsonOutput -JsonInput.StringFieldSurrogatePair.ProtobufOutput -JsonInput.StringFieldUnpairedHighSurrogate -JsonInput.StringFieldUnpairedLowSurrogate JsonInput.StringFieldUppercaseEscapeLetter -JsonInput.TimestampJsonInputLowercaseT -JsonInput.TimestampJsonInputLowercaseZ -JsonInput.TimestampJsonInputMissingT -JsonInput.TimestampJsonInputMissingZ -JsonInput.TimestampJsonInputTooLarge -JsonInput.TimestampJsonInputTooSmall JsonInput.TrailingCommaInAnObject -JsonInput.Uint32FieldNotInteger -JsonInput.Uint32FieldNotNumber -JsonInput.Uint32FieldTooLarge -JsonInput.Uint64FieldNotInteger -JsonInput.Uint64FieldNotNumber -JsonInput.Uint64FieldTooLarge JsonInput.WrapperTypesWithNullValue.JsonOutput JsonInput.WrapperTypesWithNullValue.ProtobufOutput ProtobufInput.PrematureEofBeforeKnownRepeatedValue.MESSAGE @@ -102,5 +46,3 @@ ProtobufInput.PrematureEofInPackedField.SINT64 ProtobufInput.PrematureEofInPackedField.UINT32 ProtobufInput.PrematureEofInPackedField.UINT64 ProtobufInput.PrematureEofInsideKnownRepeatedValue.MESSAGE -TimestampProtoInputTooLarge.JsonOutput -TimestampProtoInputTooSmall.JsonOutput diff --git a/conformance/failure_list_java.txt b/conformance/failure_list_java.txt index 552c0cc9..99232ad2 100644 --- a/conformance/failure_list_java.txt +++ b/conformance/failure_list_java.txt @@ -8,8 +8,6 @@ FieldMaskNumbersDontRoundTrip.JsonOutput FieldMaskPathsDontRoundTrip.JsonOutput FieldMaskTooManyUnderscore.JsonOutput JsonInput.AnyWithFieldMask.ProtobufOutput -JsonInput.AnyWithValueForInteger.JsonOutput -JsonInput.AnyWithValueForJsonObject.JsonOutput JsonInput.BoolFieldAllCapitalFalse JsonInput.BoolFieldAllCapitalTrue JsonInput.BoolFieldCamelCaseFalse @@ -30,8 +28,6 @@ JsonInput.FloatFieldInfinityNotQuoted JsonInput.FloatFieldNanNotQuoted JsonInput.FloatFieldNegativeInfinityNotQuoted JsonInput.Int32FieldLeadingZero -JsonInput.Int32FieldMinFloatValue.JsonOutput -JsonInput.Int32FieldMinValue.JsonOutput JsonInput.Int32FieldNegativeWithLeadingZero JsonInput.Int32FieldPlusSign JsonInput.Int32MapFieldKeyNotQuoted