hydromatic / morel

Standard ML interpreter, with relational extensions, implemented in Java
Apache License 2.0
291 stars 15 forks source link

detect and fix flaky tests #207

Closed Rette66 closed 7 months ago

Rette66 commented 7 months ago

Description

Root Cause

All test mentioned above has been reported as flaky when run with the NonDex tool.

These tests failed because the asserteval() method would invoke a compile process for a hard-coded code and the compile process will eventually invoke this method:

https://github.com/hydromatic/morel/blob/b3ce1d8ba7351ba6fb0dceeaf30288451fe5bf7d/src/main/java/net/hydromatic/morel/compile/Compiler.java#L710

Inside the eval execution, methods in evalEnvOf() will be invoked and a new object with the type TypeMap will be created. These methods and class utilize HashMap which do not guarantee elements' order. Therefore, unexpected results will be generated while testing. https://github.com/hydromatic/morel/blob/b3ce1d8ba7351ba6fb0dceeaf30288451fe5bf7d/src/main/java/net/hydromatic/morel/compile/CalciteCompiler.java#L777-L778 https://github.com/hydromatic/morel/blob/b3ce1d8ba7351ba6fb0dceeaf30288451fe5bf7d/src/main/java/net/hydromatic/morel/compile/TypeMap.java#L44-L48

To reproduce run:

mvn -pl <module_name> edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=<test_name>

Fix

Tests are fixed by changing HashMap to LinkedHashMap which can guarantee element order and change the expected result with the correct executing order. Besides, for testBuild testCompute in MainTest class I change the matcher to equalsUnordered to check only elements of a map instead of their orders. For dumpToStringTwice inside the SatTest I create an expected map and use assertEquals to compare the tested result and the expected result.

Key changed/added classes in this PR

julianhyde commented 7 months ago

I don't think changing typeVars from HashMap to LinkedHashMap is necessary; it is never iterated over.

We don't allow * imports even for statics, but you weren't to know; I've added a checkstyle rule.

I added a couple of fixup commits as https://github.com/julianhyde/morel/tree/207-flaky and will squash/merge if you're OK with them.

julianhyde commented 7 months ago

Merged (with slight modifications, as noted above). Thanks for the PR!

Rette66 commented 7 months ago

Merged (with slight modifications, as noted above). Thanks for the PR!

Thanks for reviewing my PR! It makes more sense with your modifications!