test: adjust flaky tests assertions on ConverterAwareMappingSpannerEntityWriterTests

[bjchris32]

What changes were proposed in this pull request?

In the test case com.google.cloud.spring.data.spanner.core.convert.ConverterAwareMappingSpannerEntityWriterTests#writeJsonArrayTest and com.google.cloud.spring.data.spanner.core.convert.ConverterAwareMappingSpannerEntityWriterTests#writeJsonTest, we changed the assertion methods on the array of JSON elements.

We used org.mockito.ArgumentCaptor to extract the argument and org.skyscreamer.jsonassert.JSONAssert to assert the JSON elements.

Why are the changes needed?

There is an implementation-dependent operation in ConverterAwareMappingSpannerEntityWriter#convertJsonToValue, which is identified by the engine NonDex.

When setting json object in ConverterAwareMappingSpannerEntityWriter will use convertJsonToValue.

spring-cloud-gcp/spring-cloud-gcp-data-spanner/src/main/java/com/google/cloud/spring/data/spanner/core/convert/ConverterAwareMappingSpannerEntityWriter.java

Line 375 in d7a10f5

String jsonString = this.spannerMappingContext.getGson().toJson(value);

However, in the test ConverterAwareMappingSpannerEntityWriterTests#writeJsonArrayTest and ConverterAwareMappingSpannerEntityWriterTests#writeJsonTest, we assert the json argument string in particular order, which would not be guaranteed in different Gson/Java version.

For example, there are possible orders in the argument of toJsonArray as below.

# possible order 1:
[{"p1":"some value","p2":"some other value"}, {"p1":"some value","p2":"some other value"}]
# possible order 2:
[{"p2":"some other value","p1":"some value"}, {"p2":"some other value","p1":"some value"}]

Error message shown in NonDex engine is as below:

[ERROR] com.google.cloud.spring.data.spanner.core.convert.ConverterAwareMappingSpannerEntityWriterTests.writeJsonArrayTest -- Time elapsed: 0.029 s <<< FAILURE!
Argument(s) are different! Wanted:
valueBinder.toJsonArray(
    [{"p1":"some value","p2":"some other value"}, {"p1":"some value","p2":"some other value"}]
);
-> at com.google.cloud.spring.data.spanner.core.convert.ConverterAwareMappingSpannerEntityWriterTests.writeJsonArrayTest(ConverterAwareMappingSpannerEntityWriterTests.java:384)
Actual invocations have different arguments:
valueBinder.toJsonArray(
    [{"p2":"some other value","p1":"some value"}, {"p2":"some other value","p1":"some value"}]
);

As a result, it would be better to change the assertion methods to prevent from the flaky behaviors.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Test Environment:

openjdk version "17.0.8.1"
Apache Maven 3.8.8
Ubuntu 20.04.6 LTS
Linux version: 5.4.0-163-generic

Set up the NonDex engine:

git clone https://github.com/kaiyaok2/NonDex
cd NonDex
git checkout support_java_17
mvn clean install -DskipTests

Run the unit test using NonDex with the following command.

cd $test_project_dir
mvn -pl spring-cloud-gcp-data-spanner edu.illinois:nondex-maven-plugin:2.1.7-SNAPSHOT:nondex -Dtest=com.google.cloud.spring.data.spanner.core.convert.ConverterAwareMappingSpannerEntityWriterTests#writeJsonArrayTest -Drat.skip=true -DnondexRuns=10 |& tee mvn-test-writeJsonArrayTest-$(date +%s).log

Please let me know if you have any concerns.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[burkedavison]

@bjchris32 : Thank you for raising this PR. I agree that this removes the potential for flakiness in the ordering of the json properties, but it also does so at the cost of increasing the tests' complexity. Has there been an issue with these tests flaking due to the way they're currently written, or are these improvements protecting against a potential problem?

At the moment, I'm not sure when balancing the additional logic with the benefit whether this is a good trade.

[bjchris32]

@bjchris32 : Thank you for raising this PR. I agree that this removes the potential for flakiness in the ordering of the json properties, but it also does so at the cost of increasing the tests' complexity. Has there been an issue with these tests flaking due to the way they're currently written, or are these improvements protecting against a potential problem?

At the moment, I'm not sure when balancing the additional logic with the benefit whether this is a good trade.

Hi @burkedavison ,
Thank you for your comments.
The failure about implementation-dependent operation is only identified by NonDex.
The flaky behavior will happen when the implementation of dependent package Gson is changed or Java version is changed.
So, this modification is for precaution for potential problems.