Skip to content

Comments

[fix][test] Made ProtobufNativeSchemaTest.testSchema order-independent#1

Open
LucasEby wants to merge 1 commit intomasterfrom
testSchemaNondeterministic
Open

[fix][test] Made ProtobufNativeSchemaTest.testSchema order-independent#1
LucasEby wants to merge 1 commit intomasterfrom
testSchemaNondeterministic

Conversation

@LucasEby
Copy link
Owner

@LucasEby LucasEby commented Oct 1, 2025

Fixes apache#24804

Motivation

In ProtobufNativeSchemaTest.testSchema, the json's contents and the FileDescriptorSet field's contents do not have a deterministic order but the hardcoded string assertion assumes a deterministic order. The json serializer did not guarantee attribute order and inside FileDescriptorSet the contents can also be in different orders due to different generation paths or environments producing the contents in different orders despite the logical content being the same. Since the original test compared the raw strings/trees "as-is", harmless re-ordering could flip the test from pass to fail without any real schema change.

Modifications

We no longer compare raw strings/trees "as-is". Instead we decode the FileDescriptorSet and compare it semantically be converting both to JSON and asserting equality using a mode that doesn't require array order to match but forbids extra fields. Next, we remove FileDescriptorSet from the outer JSON and compare the remainder with an order-agnostic (for keys) but non-extensible JSON assertion. We implemented this two stage approach because JSON assertion does not re-order the inner contents of the json values even if they can be re-ordered.

This change keeps the spirit of the original test while eliminating failures caused solely by allowed reordering.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as org.apache.pulsar.client.impl.schema.ProtobufSchemaTest#testSchema.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@LucasEby LucasEby force-pushed the testSchemaNondeterministic branch from c84ac39 to 3473283 Compare October 17, 2025 18:00
@LucasEby LucasEby force-pushed the testSchemaNondeterministic branch from 3473283 to 6f31a65 Compare October 17, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ProtobufNativeSchemaTest.testSchema order-independent

2 participants