XChange
XChange copied to clipboard
Fix flaky De/Serialization and Tests
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!
Would you please fix the branch conflicts? Then I will merge this.