Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky De/Serialization and Tests #4960

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

AmitPr
Copy link

@AmitPr AmitPr commented Nov 6, 2024

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!

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.

1 participant