Skip to content

Commit

Permalink
fix(java): Fix flakiness in ExpressionVisitorTest#testTraverseExpress…
Browse files Browse the repository at this point in the history
…ion (#1968)

## What does this PR do?

`ExpressionVisitorTest#testTraverseExpression` relies on ordered
traversal. However, the traversal depends on the order of
`getDeclaredFields()` to be deterministic (see below). The Java
specification explicitly marks `getDeclaredFields` as a function that
can return field names in any order. In the wild, this is most likely to
manifest on different architectures with different JVM versions.


https://github.com/apache/fury/blob/54b62fb6ab5d7e557131efe07c7402c885f6e7c4/java/fury-core/src/main/java/org/apache/fury/reflect/ReflectionUtils.java#L368-L381

Using [NonDex](https://github.com/TestingResearchIllinois/NonDex), we
can replicate the flaky behavior with the following command:

```
mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex -pl fury-core/ -Dcheckstyle.skip -Dmaven.javadoc.skip -Dtest=org.apache.fury.codegen.ExpressionVisitorTest
```

The fix, in this case, is to simply use HashSet equality instead of an
ordered List equality. The above NonDex command should succeed after
applying this patch.

I do, however, note that there are other flaky tests (found by running
NonDex on the entire `fury-core` project) that fail due to reliance on
e.g. `PriorityQueue#toArray`, which is also explicity marked to return
nondeterministically ordered arrays.

## Does this PR introduce any user-facing change?

No

---------

Co-authored-by: Shawn Yang <[email protected]>
  • Loading branch information
AmitPr and chaokunyang authored Dec 13, 2024
1 parent 68ca4bf commit 9be8c95
Showing 1 changed file with 8 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.apache.fury.codegen.Expression.Literal;
import org.apache.fury.reflect.ReflectionUtils;
import org.apache.fury.reflect.TypeRef;
Expand Down Expand Up @@ -63,7 +65,11 @@ public void testTraverseExpression() throws InvocationTargetException, IllegalAc
assertEquals(serializedLambda.getCapturedArgCount(), 1);
ExpressionVisitor.ExprHolder exprHolder =
(ExpressionVisitor.ExprHolder) (serializedLambda.getCapturedArg(0));
assertEquals(
expressions, Arrays.asList(forLoop, start, end, step, e1, ref, exprHolder.get("e2")));

// Traversal relies on getDeclaredFields(), nondeterministic order.
Set<Expression> expressionsSet = new HashSet<>(expressions);
Set<Expression> expressionsSet2 =
new HashSet<>(Arrays.asList(forLoop, e1, ref, exprHolder.get("e2"), end, start, step));
assertEquals(expressionsSet, expressionsSet2);
}
}

0 comments on commit 9be8c95

Please sign in to comment.