Skip to content

Comments

[fix][test] Fixed nondeterministic JSON ordering in multiple tests#3

Open
LucasEby wants to merge 1 commit intomasterfrom
fix-nondeterministicJsonOrdering
Open

[fix][test] Fixed nondeterministic JSON ordering in multiple tests#3
LucasEby wants to merge 1 commit intomasterfrom
fix-nondeterministicJsonOrdering

Conversation

@LucasEby
Copy link
Owner

@LucasEby LucasEby commented Oct 1, 2025

Fixes apache#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 LucasEby force-pushed the fix-nondeterministicJsonOrdering branch 3 times, most recently from d58211a to 6d9ed23 Compare October 3, 2025 16:36
@LucasEby LucasEby force-pushed the fix-nondeterministicJsonOrdering branch from 6d9ed23 to 7f593aa Compare October 3, 2025 21:12
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

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] Nondeterministic JSON ordering in multiple tests

2 participants