Skip to content

Commit

Permalink
Remove the entire pb_unit_tests/ directory with reflection_test_wrapper.
Browse files Browse the repository at this point in the history
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.

Since this is the last remaining test file in the pb_unit_tests directory, I
have removed the entire directory. Future UPB tests will be in the same
directory as C++ and pure python implementations, namely:
//python/google/protobuf/internal.

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 21e9aa6).

The reflection_test is a slightly more complicated test. In this commit I've
removed exemption of testParsingNestedClass in reflection_test. A follow-up
commit will ensure further conformance within the reflection_test. There are
quite a few if-conditions that were added to skip certain checks due to
nonconformant behavior -- they seem to no longer apply.

PiperOrigin-RevId: 712943652
  • Loading branch information
tonyliaoss authored and copybara-github committed Jan 7, 2025
1 parent b985164 commit 39808bc
Show file tree
Hide file tree
Showing 9 changed files with 51 additions and 136 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/test_python.yml
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ jobs:
credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
bazel-cache: python_linux/${{ matrix.type }}_${{ matrix.version }}
bazel: test ${{ matrix.targets }} ${{ matrix.flags }} --test_env=KOKORO_PYTHON_VERSION
exclude-targets: -//python/pb_unit_tests/...


macos:
Expand Down Expand Up @@ -120,4 +119,3 @@ jobs:
test ${{ matrix.targets }} ${{ matrix.flags }}
--test_env=KOKORO_PYTHON_VERSION=${{ matrix.version }}
--macos_minimum_os=11.0
exclude-targets: -//python/pb_unit_tests/...
12 changes: 3 additions & 9 deletions .github/workflows/test_upb.yml
Original file line number Diff line number Diff line change
Expand Up @@ -264,21 +264,15 @@ jobs:
- name: Install Protobuf Test Wheel
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
run: pip install -vvv --no-index --find-links wheels protobuftests
- name: Run legacy unit tests
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
# Legacy upb tests use a "test wrapper" and get installed under pb_unit_tests.
run: |
PB_UNIT_TESTS=$(pip show -f protobuftests | grep pb_unit_tests.*py$ | sed 's,/,.,g' | sed 's,\\,.,g' | sed -E 's,.py$,,g')
for test in $PB_UNIT_TESTS; do
python -m unittest -v $test
done
- name: Run unit tests
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
# Newer upb tests are in the standard google.protobuf.internal path.
# We will eventually make this into a wildcard rule once all tests
# have been migrated to be compatible with upb.
# TODO: b/378725969 - Use wildcard expansion to ensure that we don't
# accidentally miss test coverage.
run: |
TESTS=(message_test message_factory_test descriptor_test proto_builder_test descriptor_pool_test generator_test)
TESTS=(message_test message_factory_test descriptor_test proto_builder_test descriptor_pool_test generator_test reflection_test)
for test in ${TESTS[@]}; do
python -m unittest -v google.protobuf.internal.${test}
done
Expand Down
1 change: 0 additions & 1 deletion python/dist/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,6 @@ py_wheel(
"//:python_common_test_protos",
"//:python_specific_test_protos",
"//:python_test_srcs",
"//python/pb_unit_tests:test_files",
"//src/google/protobuf:testdata",
"@com_google_absl_py//absl/testing:parameterized",
],
Expand Down
1 change: 0 additions & 1 deletion python/google/protobuf/internal/numpy/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ internal_py_test(
srcs = ["numpy_test.py"],
visibility = [
"//python:__pkg__",
"//python/pb_unit_tests:__pkg__",
],
deps = [
requirement("numpy"),
Expand Down
60 changes: 48 additions & 12 deletions python/google/protobuf/internal/reflection_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1607,6 +1607,11 @@ def testExtensionDelete(self):
del extendee_proto.Extensions[extension_int32]
self.assertEqual(len(extendee_proto.Extensions), 0)

@unittest.skipIf(
api_implementation.Type() == 'upb',
'This test relies on a specific iteration order for extensions, '
'which is not reasonable to guarantee.',
)
def testExtensionIter(self):
extendee_proto = more_extensions_pb2.ExtendedMessage()

Expand Down Expand Up @@ -2740,6 +2745,11 @@ def testParseTruncated(self):
self.assertRaises(message.DecodeError, unknown_fields._InternalParse,
serialized, 0, truncation_point)

@unittest.skipIf(
api_implementation.Type() == 'upb',
'This test relies on a specific iteration order for extensions, '
'which is not reasonable to guarantee.',
)
def testCanonicalSerializationOrder(self):
proto = more_messages_pb2.OutOfOrderFields()
# These are also their tag numbers. Even though we're setting these in
Expand All @@ -2765,6 +2775,11 @@ def testCanonicalSerializationOrder(self):
self.assertEqual((5, wire_format.WIRETYPE_VARINT), ReadTag())
self.assertEqual(5, d.ReadSInt32())

@unittest.skipIf(
api_implementation.Type() == 'upb',
'This test relies on a specific iteration order for extensions, '
'which is not reasonable to guarantee.',
)
def testCanonicalSerializationOrderSameAsCpp(self):
# Copy of the same test we use for C++.
proto = unittest_pb2.TestFieldOrderings()
Expand Down Expand Up @@ -3141,19 +3156,40 @@ def testExtensionFieldNumbers(self):

def testFieldProperties(self):
cls = unittest_pb2.TestAllTypes
self.assertIs(cls.optional_int32.DESCRIPTOR,
cls.DESCRIPTOR.fields_by_name['optional_int32'])
self.assertEqual(cls.OPTIONAL_INT32_FIELD_NUMBER,
cls.optional_int32.DESCRIPTOR.number)
self.assertIs(cls.optional_nested_message.DESCRIPTOR,
cls.DESCRIPTOR.fields_by_name['optional_nested_message'])
self.assertEqual(cls.OPTIONAL_NESTED_MESSAGE_FIELD_NUMBER,
cls.optional_nested_message.DESCRIPTOR.number)
self.assertIs(cls.repeated_int32.DESCRIPTOR,
cls.DESCRIPTOR.fields_by_name['repeated_int32'])
self.assertEqual(cls.REPEATED_INT32_FIELD_NUMBER,
cls.repeated_int32.DESCRIPTOR.number)
if api_implementation.Type() == 'upb':
# Class accessors are not implemented in upb.
with self.assertRaises(AttributeError) as e:
# Try to access the descriptor of the field 'optional_int32'
cls.optional_int32.DESCRIPTOR
self.assertEquals('optional_int32', str(e.exception))
else:
self.assertIs(
cls.optional_int32.DESCRIPTOR,
cls.DESCRIPTOR.fields_by_name['optional_int32'],
)
self.assertEqual(
cls.OPTIONAL_INT32_FIELD_NUMBER, cls.optional_int32.DESCRIPTOR.number
)
self.assertIs(
cls.optional_nested_message.DESCRIPTOR,
cls.DESCRIPTOR.fields_by_name['optional_nested_message'],
)
self.assertEqual(
cls.OPTIONAL_NESTED_MESSAGE_FIELD_NUMBER,
cls.optional_nested_message.DESCRIPTOR.number,
)
self.assertIs(
cls.repeated_int32.DESCRIPTOR,
cls.DESCRIPTOR.fields_by_name['repeated_int32'],
)
self.assertEqual(
cls.REPEATED_INT32_FIELD_NUMBER, cls.repeated_int32.DESCRIPTOR.number
)

@unittest.skipIf(
api_implementation.Type() == 'upb',
'Class accessors are not implemented in upb: see testFieldProperties.',
)
def testFieldDataDescriptor(self):
msg = unittest_pb2.TestAllTypes()
msg.optional_int32 = 42
Expand Down
22 changes: 0 additions & 22 deletions python/pb_unit_tests/BUILD

This file was deleted.

11 changes: 0 additions & 11 deletions python/pb_unit_tests/README.md

This file was deleted.

25 changes: 0 additions & 25 deletions python/pb_unit_tests/pyproto_test_wrapper.bzl

This file was deleted.

53 changes: 0 additions & 53 deletions python/pb_unit_tests/reflection_test_wrapper.py

This file was deleted.

0 comments on commit 39808bc

Please sign in to comment.