Skip to content

Conversation

@LucasEby
Copy link
Contributor

Fixes #24967

Motivation

This fix is the same as this fix which was previously accepted and merged: #24821

Each of the below tests incorrectly assumes that the json key-value pairs being asserted would have a deterministic order. The order of key-value pairs is not guaranteed in JSON, however. As a result, the ordering can change due to different environments producing the contents in different orders despite the logical contents being the same. Harmless re-ordering could flip the tests from pass to fail despite the data being semantically the same:

  • org.apache.pulsar.client.impl.schema.SchemaInfoTest$SchemaInfoBuilderTest.testNullPropertyValue
  • org.apache.pulsar.client.impl.schema.SchemaInfoTest.testSchemaInfoToString

Modifications

We no longer compare raw strings "as-is" with Assert.assertEquals. Instead, we compare the json structures with JsonAssert.assertEquals(expectedJson, actualJson, JSONCompareMode) which parses both inputs into JSON trees. It treats these JSON trees as unordered collections of key-value pairs when determining if they are equal so differences in property order no longer matter. This ensures the tests pass consistently, even when the serializer outputs fields in a different order.

In essence, these changes keep the spirit of the original tests 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.SchemaInfoTest$SchemaInfoBuilderTest.testNullPropertyValue
  • org.apache.pulsar.client.impl.schema.SchemaInfoTest.testSchemaInfoToString

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#9

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2025
@Technoboy- Technoboy- added this to the 4.2.0 milestone Nov 11, 2025
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.41%. Comparing base (c4f125c) to head (43899e9).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #24969       +/-   ##
=============================================
+ Coverage     38.77%   74.41%   +35.64%     
- Complexity    13380    34040    +20660     
=============================================
  Files          1856     1920       +64     
  Lines        145342   150062     +4720     
  Branches      16886    17404      +518     
=============================================
+ Hits          56353   111674    +55321     
+ Misses        81459    29512    -51947     
- Partials       7530     8876     +1346     
Flag Coverage Δ
inttests 26.25% <ø> (-0.16%) ⬇️
systests 22.87% <ø> (+0.02%) ⬆️
unittests 73.93% <ø> (+38.93%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1419 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.

@lhotari lhotari merged commit 8f357b0 into apache:master Nov 11, 2025
136 of 140 checks passed
lhotari pushed a commit that referenced this pull request Nov 11, 2025
lhotari pushed a commit that referenced this pull request Nov 11, 2025
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 13, 2025
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Nondeterministic Ordering in SchemaInfoTest

4 participants