Skip to content

Conversation

LucasEby
Copy link

@LucasEby LucasEby commented Oct 4, 2025

Fixes #24820

Motivation

Each of the tests below were written with the assumption that the json key-value pairs would have a deterministic order (by comparing json results vs hard coded strings). The order of key-value pairs is not guaranteed in JSON files or JSON objects, though. As a result, the ordering can change due to different environments producing the contents in different orders despite the logical contents being the same. Since each of the tests below compare the raw strings/trees "as-is", harmless re-ordering could flip the test from pass to fail despite the data being semantically the same.

  • org.apache.pulsar.common.policies.data.NamespaceOwnershipStatusTest#testSerialization
  • org.apache.pulsar.common.policies.impl.NamespaceIsolationPoliciesTest#testJsonSerialization
  • org.apache.pulsar.functions.utils.FunctionConfigUtilsTest#testConvertBackFidelity
  • org.apache.pulsar.functions.utils.SinkConfigUtilsTest#testConvertBackFidelity
  • org.apache.pulsar.functions.utils.SourceConfigUtilsTest#testBatchConfigMergeEqual
  • org.apache.pulsar.io.elasticsearch.ElasticSearchExtractTest#testGenericRecord
  • org.apache.pulsar.io.kinesis.UtilsTest#testKeyValueSerializeNoValue.

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.common.policies.data.NamespaceOwnershipStatusTest#testSerialization, org.apache.pulsar.common.policies.impl.NamespaceIsolationPoliciesTest#testJsonSerialization, org.apache.pulsar.functions.utils.FunctionConfigUtilsTest#testConvertBackFidelity, org.apache.pulsar.functions.utils.SinkConfigUtilsTest#testConvertBackFidelity, org.apache.pulsar.functions.utils.SourceConfigUtilsTest#testBatchConfigMergeEqual , org.apache.pulsar.io.elasticsearch.ElasticSearchExtractTest#testGenericRecord, and org.apache.pulsar.io.kinesis.UtilsTest#testKeyValueSerializeNoValue.

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 4, 2025
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, thanks for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Nondeterministic JSON ordering in multiple tests
2 participants