Hi,

The test customGsonBuilder() is non deterministic or flaky. Gson.toJson() function does not promise any ordering of fields in the returned JSON string (at least not by itself). Therefore, the result of gson.toJson(new DataObject()) could be {data: 1, owner: null} or {owner: null, data: 1}. Both of them are the same JSON objects, and ideally, application should not differentiate based on the ordering of JSON fields. These tests may start to fail in the near future and ring a false alarm. Here's a SO post for more details: https://stackoverflow.com/questions/6365851/how-to-keep-fields-sequence-in-gson-serialization.

I ran the test again after my changes to confirm it passes: ./gradlew :spring-boot-project:spring-boot-autoconfigure:test --tests org.springframework.boot.autoconfigure.gson.GsonAutoConfigurationTests.customGsonBuilder.

Please let me know if you have any questions. There are some more flaky tests in this repository. If you like this fix, I can open more PRs for the other flaky tests.

Comment From: pivotal-cla

@chessvivek Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Comment From: pivotal-cla

@chessvivek Thank you for signing the Contributor License Agreement!

Comment From: snicoll

Thanks for the PR.

There are some more flaky tests in this repository.

Can you share more details about that?

Comment From: chessvivek

Thanks for the PR.

There are some more flaky tests in this repository.

Can you share more details about that?

These are some of the other flaky tests that I have found in this project:

org.springframework.boot.autoconfigure.websocket.servlet.WebSocketMessagingAutoConfigurationTests.basicMessagingWithJsonResponse org.springframework.boot.context.properties.bind.CollectionBinderTests.bindToSetShouldNotAllowDuplicateValues org.springframework.boot.diagnostics.analyzer.BeanCurrentlyInCreationFailureAnalyzerTests.cycleWithAutowiredFields

I have found these flaky tests using the NonDex tool (https://github.com/TestingResearchIllinois/NonDex), created at the University of Illinois. I have not yet looked into the source of flakiness for these tests, or how to fix them. But I can do that once I am done with this PR.

Comment From: chessvivek

There's also this nice library called JSONAssert - https://github.com/skyscreamer/JSONassert that allows us to do JSON to JSON comparison on two JSON strings. In the long run, I think using this library is a cleaner way to prevent any non-determinism from JSON string comparison. I just did not use this library because it will add an extra dependency. But if you would like, I can do definitely do that.

Comment From: wilkinsona

Yes, please do use JSONAssert. It is already included in Boot’s dependency management.

Comment From: chessvivek

Yes, please do use JSONAssert. It is already included in Boot’s dependency management.

Thanks for letting me know. I have revised the changes to use the JSONAssert library.

Comment From: wilkinsona

@chessvivek Thanks very much for making your first contribution to Spring Boot.

WebSocketMessagingAutoConfigurationTests.basicMessagingWithJsonResponse

This is the same problem as we had in GsonAutoConfigurationTests.customGsonBuilder. I have fixed it as part of merging this PR. Thanks for bringing it to our attention.

CollectionBinderTests.bindToSetShouldNotAllowDuplicateValues

This looks like a false positive. A LinkedHashSet is used so the ordering is deterministic.

BeanCurrentlyInCreationFailureAnalyzerTests.cycleWithAutowiredFields

I suspect this may be a false positive as well, although it's hard to say without knowing why the tool believes it to be flaky. IIRC, we've never seen this test flake on CI or in local builds.

Comment From: chessvivek

@chessvivek Thanks very much for making your first contribution to Spring Boot.

WebSocketMessagingAutoConfigurationTests.basicMessagingWithJsonResponse

This is the same problem as we had in GsonAutoConfigurationTests.customGsonBuilder. I have fixed it as part of merging this PR. Thanks for bringing it to our attention.

CollectionBinderTests.bindToSetShouldNotAllowDuplicateValues

This looks like a false positive. A LinkedHashSet is used so the ordering is deterministic.

BeanCurrentlyInCreationFailureAnalyzerTests.cycleWithAutowiredFields

I suspect this may be a false positive as well, although it's hard to say without knowing why the tool believes it to be flaky. IIRC, we've never seen this test flake on CI or in local builds.

Thanks @wilkinsona for accepting my fix, and also investigating/fixing other tests. I see that you enabled the strict mode by passing true as the third argument in JSONAssert.assertEquals. However, the order is only ignored when you disable the strict mode. In the documentation, it says - When strict is set to false (recommended), it forgives reordering data and extending results (as long as all the expected elements are there), making tests less brittle.

If you just want to forgive reordering data and not extending results (which is what I think you want), you can also pass in JSONCompareMode.NON_EXTENSIBLE as the third argument. The third argument can also take a JSONCompareMode, the documenation for which you can see here - https://jsonassert.skyscreamer.org/apidocs/index.html

Comment From: wilkinsona

As there are no arrays involved, I believe strict mode meets our needs. It is described as follows:

Strict tests require all of the elements requested to be returned, and only those elements (ie, the tests are non-extensible). Arrays of elements must be returned in the same order as expected.

I verified this by temporarily changing the order of the expected json and the test passed.