Skip to content

Conversation

LucasEby
Copy link
Contributor

@LucasEby LucasEby commented Oct 1, 2025

Fixes #24806

Motivation

In ProtobufSchemaTest.testParsingInfoProperty, the json's contents and the PARSING_INFO field's contents do not have a deterministic order but the hardcoded string assertion assumes a deterministic order.

Serializing a map to a JSON with new ObjectMapper().writeValueAsString(schema.getSchemaInfo().getProperties()) can change the order of the keys since attribute order is not guaranteed in JSON. Additionally the inner JSON string order in PARSING_INFO can vary for the same reason. The ordering in both cases can change due to different environments producing the contents in different orders despite the logical contents being the same. Since the test compares the raw strings/trees "as-is", harmless re-ordering could flip the test from pass to fail despite the data being semantically the same.

Modifications

We no longer compare raw strings/trees "as-is". Instead, we compare a json structure that we derive. When we normalize the properties, we copy every key/value from the properties map so nothing at the top level is ignored. For __PARSING_INFO__ only we parse every string value into an ArrayNode, sort the array deterministically by (number, name) to remove element-order nondeterminism, and then store it back as a JSON array (instead of a string). For all other keys we keep their string values as-is.

When performing the Assert.assertEquals assertion, it is performed with the JsonNode equals method which is an important subtlety. This is because it compares field names and values not memory addresses. Additionally field order is not considered. This is the reason why we treated the __PARSING_INFO__ field differently before because this assertion is sensitive to the order of the contents of __PARSING_INFO__ (it does an as-is comparison instead of allowing different orders).

This change keeps the spirit of the original test while eliminating failures caused solely by allowed (but previously unexpected) 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#testParsingInfoProperty.

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: LucasEby#2

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 1, 2025
@LucasEby LucasEby force-pushed the testParsingInfoPropertyNondeterministic branch from 2e8176c to 7003e52 Compare October 1, 2025 17:50
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LucasEby LucasEby force-pushed the testParsingInfoPropertyNondeterministic branch 2 times, most recently from 353d0be to ebc185d Compare October 2, 2025 16:34
@LucasEby
Copy link
Contributor Author

LucasEby commented Oct 2, 2025

I apologize, I forgot to enable the CI checks on my local fork after creating it. I fixed the formatting issues but the code failed some other CI checks on my fork so I'm looking into that right now. I will update this once I've gotten all of the CI checks to pass on my fork.

@LucasEby LucasEby force-pushed the testParsingInfoPropertyNondeterministic branch 5 times, most recently from c8176ad to ae0b534 Compare October 3, 2025 16:16
@LucasEby LucasEby force-pushed the testParsingInfoPropertyNondeterministic branch from ae0b534 to 7ad6d73 Compare October 3, 2025 16:30
@LucasEby
Copy link
Contributor Author

LucasEby commented Oct 3, 2025

@lhotari All checks passed on my fork: LucasEby#2
The code should be good to merge

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.24%. Comparing base (cc0eef9) to head (7ad6d73).
⚠️ Report is 21 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24807      +/-   ##
============================================
- Coverage     74.34%   74.24%   -0.10%     
+ Complexity    33842    33769      -73     
============================================
  Files          1912     1912              
  Lines        149072   149072              
  Branches      17299    17299              
============================================
- Hits         110833   110684     -149     
- Misses        29440    29555     +115     
- Partials       8799     8833      +34     
Flag Coverage Δ
inttests 26.52% <ø> (+0.28%) ⬆️
systests 22.75% <ø> (-0.08%) ⬇️
unittests 73.77% <ø> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 92 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/test doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ProtobufSchemaTest.testParsingInfoProperty order-independent

3 participants