From 7b71e08e3c22d1d1c39d42fa60da92a36a43ef5d Mon Sep 17 00:00:00 2001 From: Tony Liao Date: Fri, 3 Jan 2025 13:18:41 -0800 Subject: [PATCH] Remove descriptor_pool_test_wrapper for python's descriptor_pool_test. The test wrappers were another way to document nonconformant behaviour between different python backends. We can achieve the same by removing the wrapper script and adding an if-condition in the test itself based on api_implementation.Type(). Since we already do that for nonconformance between pure Python vs. C++ backends, this change makes it easier to look for UPB nonconformance instead of going through another layer of indirection. Temporarily, we will need to hardcode the migrated test name in test_upb.yml because not all tests under google.protobuf.internal support UPB yet. (UPB testing for selected tests are added in https://github.com/protocolbuffers/protobuf/commit/21e9aa6cac24870edf050eb0ff9b591380340c4d). PiperOrigin-RevId: 711837521 --- .github/workflows/test_upb.yml | 2 +- .../protobuf/internal/descriptor_pool_test.py | 134 +++++++++++++----- python/pb_unit_tests/BUILD | 2 - .../descriptor_pool_test_wrapper.py | 37 ----- 4 files changed, 101 insertions(+), 74 deletions(-) delete mode 100644 python/pb_unit_tests/descriptor_pool_test_wrapper.py diff --git a/.github/workflows/test_upb.yml b/.github/workflows/test_upb.yml index 2edc19d22917f..d86b422899acd 100644 --- a/.github/workflows/test_upb.yml +++ b/.github/workflows/test_upb.yml @@ -278,7 +278,7 @@ jobs: # We will eventually make this into a wildcard rule once all tests # have been migrated to be compatible with upb. run: | - TESTS=(message_test message_factory_test descriptor_test proto_builder_test) + TESTS=(message_test message_factory_test descriptor_test proto_builder_test descriptor_pool_test) for test in ${TESTS[@]}; do python -m unittest -v google.protobuf.internal.${test} done diff --git a/python/google/protobuf/internal/descriptor_pool_test.py b/python/google/protobuf/internal/descriptor_pool_test.py index 89aa15b3a47f0..a39099f8489d7 100644 --- a/python/google/protobuf/internal/descriptor_pool_test.py +++ b/python/google/protobuf/internal/descriptor_pool_test.py @@ -718,6 +718,87 @@ def setUp(self): no_package_pb2.DESCRIPTOR.serialized_pb)) self.pool = descriptor_pool.DescriptorPool(descriptor_db=self.db) + # TODO: b/387527786 - This is a helper function for testErrorCollector to + # capture some of the nonconformant, C++-specific behavior. + def assertCppErrorCollectorCorrect(self): + self.assertEqual(api_implementation.Type(), 'cpp') + error_msg = ( + 'Invalid proto descriptor for file "error_file":\\n ' + 'collector.ErrorMessage.nested_message_field: "SubMessage" ' + 'is not defined.\\n collector.ErrorMessage.MyOneof: Oneof ' + "must have at least one field.\\n'" + ) + with self.assertRaises(KeyError) as exc: + self.pool.FindMessageTypeByName('collector.ErrorMessage') + self.assertEqual( + str(exc.exception), + "'Couldn\\'t build file for message collector.ErrorMessage\\n" + + error_msg, + ) + + with self.assertRaises(KeyError) as exc: + self.pool.FindFieldByName('collector.ErrorMessage.nested_message_field') + self.assertEqual( + str(exc.exception), + "'Couldn\\'t build file for field" + ' collector.ErrorMessage.nested_message_field\\n' + + error_msg, + ) + + with self.assertRaises(KeyError) as exc: + self.pool.FindEnumTypeByName('collector.MyEnum') + self.assertEqual( + str(exc.exception), + "'Couldn\\'t build file for enum collector.MyEnum\\n" + error_msg, + ) + + with self.assertRaises(KeyError) as exc: + self.pool.FindFileContainingSymbol('collector.MyEnumValue') + self.assertEqual( + str(exc.exception), + "'Couldn\\'t build file for symbol collector.MyEnumValue\\n" + + error_msg, + ) + + with self.assertRaises(KeyError) as exc: + self.pool.FindOneofByName('collector.ErrorMessage.MyOneof') + self.assertEqual( + str(exc.exception), + "'Couldn\\'t build file for oneof collector.ErrorMessage.MyOneof\\n" + + error_msg, + ) + + # TODO: b/387527786 - This is a helper function for testErrorCollector to + # capture some of the nonconformant, UPB-specific behavior. + def assertUpbErrorCollectorCorrect(self): + self.assertEqual(api_implementation.Type(), 'upb') + # Nonconformance: compared with C++, UPB will have less descriptive + # error messages, and raise TypeError instead of KeyError. + error_msg = ( + "Couldn't build proto file into descriptor pool: " + "couldn't resolve name 'SubMessage'" + ) + + with self.assertRaises(TypeError) as exc: + self.pool.FindMessageTypeByName('collector.ErrorMessage') + self.assertEqual(str(exc.exception), error_msg) + + with self.assertRaises(TypeError) as exc: + self.pool.FindFieldByName('collector.ErrorMessage.nested_message_field') + self.assertEqual(str(exc.exception), error_msg) + + with self.assertRaises(TypeError) as exc: + self.pool.FindEnumTypeByName('collector.MyEnum') + self.assertEqual(str(exc.exception), error_msg) + + with self.assertRaises(TypeError) as exc: + self.pool.FindFileContainingSymbol('collector.MyEnumValue') + self.assertEqual(str(exc.exception), error_msg) + + with self.assertRaises(TypeError) as exc: + self.pool.FindOneofByName('collector.ErrorMessage.MyOneof') + self.assertEqual(str(exc.exception), error_msg) + def testErrorCollector(self): file_proto = descriptor_pb2.FileDescriptorProto() file_proto.package = 'collector' @@ -739,11 +820,20 @@ def testErrorCollector(self): enum_value.number = 0 self.db.Add(file_proto) - self.assertRaisesRegex(KeyError, 'SubMessage', - self.pool.FindMessageTypeByName, - 'collector.ErrorMessage') - self.assertRaisesRegex(KeyError, 'SubMessage', self.pool.FindFileByName, - 'error_file') + # Nonconformance: UPB will raise a TypeError whereas other implementations + # will raise KeyError when SubMessage cannot be indexed. + # TODO: b/387527786 - Fix this nonconformance between (cpp+python)/upb. + error_type = TypeError if api_implementation.Type() == 'upb' else KeyError + self.assertRaisesRegex( + error_type, + 'SubMessage', + self.pool.FindMessageTypeByName, + 'collector.ErrorMessage', + ) + self.assertRaisesRegex( + error_type, 'SubMessage', self.pool.FindFileByName, 'error_file' + ) + with self.assertRaises(KeyError) as exc: self.pool.FindFileByName('none_file') self.assertIn(str(exc.exception), ('\'none_file\'', @@ -755,36 +845,12 @@ def testErrorCollector(self): # called the first time, a KeyError will be raised but call the find # method later will return a descriptor which is not build. # TODO: fix pure python to revert the load if file can not be build + # TODO: b/387527786 - Fix this nonconformance between python/cpp/upb. if api_implementation.Type() != 'python': - error_msg = ('Invalid proto descriptor for file "error_file":\\n ' - 'collector.ErrorMessage.nested_message_field: "SubMessage" ' - 'is not defined.\\n collector.ErrorMessage.MyOneof: Oneof ' - 'must have at least one field.\\n\'') - with self.assertRaises(KeyError) as exc: - self.pool.FindMessageTypeByName('collector.ErrorMessage') - self.assertEqual(str(exc.exception), '\'Couldn\\\'t build file for ' - 'message collector.ErrorMessage\\n' + error_msg) - - with self.assertRaises(KeyError) as exc: - self.pool.FindFieldByName('collector.ErrorMessage.nested_message_field') - self.assertEqual(str(exc.exception), '\'Couldn\\\'t build file for field' - ' collector.ErrorMessage.nested_message_field\\n' - + error_msg) - - with self.assertRaises(KeyError) as exc: - self.pool.FindEnumTypeByName('collector.MyEnum') - self.assertEqual(str(exc.exception), '\'Couldn\\\'t build file for enum' - ' collector.MyEnum\\n' + error_msg) - - with self.assertRaises(KeyError) as exc: - self.pool.FindFileContainingSymbol('collector.MyEnumValue') - self.assertEqual(str(exc.exception), '\'Couldn\\\'t build file for symbol' - ' collector.MyEnumValue\\n' + error_msg) - - with self.assertRaises(KeyError) as exc: - self.pool.FindOneofByName('collector.ErrorMessage.MyOneof') - self.assertEqual(str(exc.exception), '\'Couldn\\\'t build file for oneof' - ' collector.ErrorMessage.MyOneof\\n' + error_msg) + if api_implementation.Type() == 'cpp': + self.assertCppErrorCollectorCorrect() + else: + self.assertUpbErrorCollectorCorrect() class ProtoFile(object): diff --git a/python/pb_unit_tests/BUILD b/python/pb_unit_tests/BUILD index 77d74a186a748..8f76ba717e01c 100644 --- a/python/pb_unit_tests/BUILD +++ b/python/pb_unit_tests/BUILD @@ -11,8 +11,6 @@ package(default_applicable_licenses = ["//:license"]) licenses(["notice"]) -pyproto_test_wrapper(name = "descriptor_pool_test") - pyproto_test_wrapper(name = "generator_test") pyproto_test_wrapper(name = "reflection_test") diff --git a/python/pb_unit_tests/descriptor_pool_test_wrapper.py b/python/pb_unit_tests/descriptor_pool_test_wrapper.py deleted file mode 100644 index 6397250fa8f09..0000000000000 --- a/python/pb_unit_tests/descriptor_pool_test_wrapper.py +++ /dev/null @@ -1,37 +0,0 @@ -# Protocol Buffers - Google's data interchange format -# Copyright 2023 Google LLC. All rights reserved. -# https://developers.google.com/protocol-buffers/ -# -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are -# met: -# -# * Redistributions of source code must retain the above copyright -# notice, this list of conditions and the following disclaimer. -# * Redistributions in binary form must reproduce the above -# copyright notice, this list of conditions and the following disclaimer -# in the documentation and/or other materials provided with the -# distribution. -# * Neither the name of Google LLC nor the names of its -# contributors may be used to endorse or promote products derived from -# this software without specific prior written permission. -# -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -import unittest -from google.protobuf.internal.descriptor_pool_test import * - -SecondaryDescriptorFromDescriptorDB.testErrorCollector.__unittest_expecting_failure__ = True - -if __name__ == '__main__': - unittest.main(verbosity=2)