XChange icon indicating copy to clipboard operation
XChange copied to clipboard

Fix flaky De/Serialization and Tests

Open AmitPr opened this issue 1 year ago • 1 comments

Seeing two types of flakiness that this PR attempts to address: JSON String Equality assertions, and JSON property ordering flakiness.

Solution:

Firstly, across the XChange test suite, there are several instances of tests that validate a JSON object. The JSON specification is inherently unordered, and thus, a simple string equality may fail if because we rely upon jackson producing ordered JSON. jackson internally uses a Hash-based datastructure, so iteration order is not guaranteed to be deterministic. For example:

// Flaky!
Assert.assertEquals(
        "{\"op\":\"subscribe\",\"args\":[\"name\"]}",
        service.getSubscribeMessage("name") // Could be "{\"args\":[\"name\"],\"op\":\"subscribe\"}"
);
// Works:
Assert.assertEquals(
        objectMapper.readTree("{\"op\":\"subscribe\",\"args\":[\"name\"]}"),
        objectMapper.readTree(service.getSubscribeMessage("name"))
);

Second: Many of the DTO objects that are parsed by XChange via jackson rely on ordered arrays, e.g. a [price, size] array. However, we cannot rely upon the order that class fields are declared in the class source, as Java Class' getDeclaredFields() is explicitly nondeterministic. Thus, annotating DTO objects that have JsonFormat.Shape.ARRAY with a @JsonPropertyOrder (doc) is best practice and prevents deserialization errors (or worse -- incorrectly parsed data that doesn't throw an error). An example:

@JsonFormat(shape = JsonFormat.Shape.ARRAY)
@JsonPropertyOrder({"orderId", "price", "amount"}) /* NEW */
public class BitfinexTradingRawOrder {
  long orderId;
  BigDecimal price;
  BigDecimal amount;
}

// Before JsonPropertyOrder: Parsing [1, 2.0, 3.0] could result in:
// price: 2.0, amount: 3.0 (correct) or
// Thrown Error (parsing long from decimal type)
// price: 3.0, amount: 2.0 (silent failure)

Reproduction:

These are most likely to cause test failures / flaky behavior if XChange is compiled and tested with different JVM/JDK distributions running on different architectures, so is somewhat of a preemptive measure. The tests can be shown to be flaky via NonDex and the following command:

# Should pass: (Using coinegg module for example. Many modules are applicable)
mvn test -pl xchange-coinegg
# Should fail before this PR, pass after this PR:
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -pl xchange-coinegg -DnondexRuns=10

Happy to iterate on this!

AmitPr avatar Nov 06 '24 08:11 AmitPr

Would you please fix the branch conflicts? Then I will merge this.

timmolter avatar Mar 23 '25 15:03 timmolter