[incubator-kie-issues#6118] Fix flaky tests in AccumulateTest

[everbrightw]

Description

I encountered non-deterministic behavior while running the following tests using NonDex:

1.	org.drools.model.codegen.execmodel.AccumulateTest.testInlineAccumulateWithAnd()
2.	org.drools.model.codegen.execmodel.AccumulateTest.testBindingOrderWithInlineAccumulateAndLists()

Steps to reproduce

NonDex: https://github.com/TestingResearchIllinois/NonDex
Run the tests with NonDex:

mvn -pl drools-model/drools-model-codegen edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.drools.model.codegen.execmodel.AccumulateTest#testInlineAccumulateWithAnd

The error message shows:

Click to view

[ERROR] Failures:
[ERROR]   AccumulateTest.testInlineAccumulateWithAnd:2752->BaseModelTest.getKieSession:105->BaseModelTest.getKieSession:109->BaseModelTest.getKieContainer:113->BaseModelTest.getKieContainer:120->BaseModelTest.createKieBuilder:131->BaseModelTest.createKieBuilder:156 [Message [id=1, level=ERROR, path=src/main/java/defaultpkg/RulesFCB815F80535171A856D047578567E4ARuleMethods0.java, line=57, column=7042
   text=The method execute(Block2<List,BigDecimal>) in the type ConsequenceBuilder._2<List,BigDecimal> is not applicable for the arguments (LambdaConsequence5A97B91F758DE5E929A604F8AF538731)], Message [id=2, level=ERROR, path=src/main/java/defaultpkg/RulesFCB815F80535171A856D047578567E4ARuleMethods0.java, line=0, column=0
   text=Java source of src/main/java/defaultpkg/RulesFCB815F80535171A856D047578567E4ARuleMethods0.java in error:
package defaultpkg;

import org.drools.modelcompiler.dsl.pattern.D;
import org.drools.model.Index.ConstraintType;
import org.drools.model.codegen.execmodel.AccumulateTest.Order;
import java.math.BigDecimal;
import org.drools.model.codegen.execmodel.AccumulateTest.Car;
import static defaultpkg.RulesFCB815F80535171A856D047578567E4A.*;
import static defaultpkg.RulesFCB815F80535171A856D047578567E4A.*;

public class RulesFCB815F80535171A856D047578567E4ARuleMethods0 {

    /**
     * Rule name: R
     */
    public static org.drools.model.Rule rule_R() {
        final org.drools.model.Variable<java.math.BigDecimal> var_$total = D.declarationOf(java.math.BigDecimal.class,
                                                                                           DomainClassesMetadataFCB815F80535171A856D047578567E4A.java_math_BigDecimal_Metadata_INSTANCE,
                                                                                           "$total");
        final org.drools.model.Variable<org.drools.model.codegen.execmodel.AccumulateTest.Car> var_$car_1_sCoPe = D.declarationOf(org.drools.model.codegen.execmodel.AccumulateTest.Car.class,
                                                                                                                                  DomainClassesMetadataFCB815F80535171A856D047578567E4A.org_drools_model_codegen_execmodel_AccumulateTest_Car_Metadata_INSTANCE,
                                                                                                                                  "$car_1_sCoPe");
        final org.drools.model.Variable<org.drools.model.codegen.execmodel.AccumulateTest.Order> var_GENERATED_$pattern_Order$2$ = D.declarationOf(org.drools.model.codegen.execmodel.AccumulateTest.Order.class,
                                                                                                                                                   DomainClassesMetadataFCB815F80535171A856D047578567E4A.org_drools_model_codegen_execmodel_AccumulateTest_Order_Metadata_INSTANCE,
                                                                                                                                                   "GENERATED_$pattern_Order$2$");
        final org.drools.model.Variable<java.math.BigDecimal> var_$price_1_sCoPe = D.declarationOf(java.math.BigDecimal.class,
                                                                                                   DomainClassesMetadataFCB815F80535171A856D047578567E4A.java_math_BigDecimal_Metadata_INSTANCE,
                                                                                                   "$price_1_sCoPe");
        final org.drools.model.Variable<java.math.BigDecimal> var_total = D.declarationOf(java.math.BigDecimal.class,
                                                                                          DomainClassesMetadataFCB815F80535171A856D047578567E4A.java_math_BigDecimal_Metadata_INSTANCE,
                                                                                          "total");
        org.drools.model.Rule rule = D.rule("R")
                                      .build(D.pattern(var_$total),
                                             D.accumulate(D.and(D.pattern(var_$car_1_sCoPe).expr("GENERATED_9B0C55783F006C0E62C1D71652E2D1F6",
                                                                                                 defaultpkg.P30.LambdaPredicate30E28C5B6FC1EE01A1A5E80C4BD31C35.INSTANCE,
                                                                                                 D.alphaIndexedBy(boolean.class,
                                                                                                                  org.drools.model.Index.ConstraintType.EQUAL,
                                                                                                                  DomainClassesMetadataFCB815F80535171A856D047578567E4A.org_drools_model_codegen_execmodel_AccumulateTest_Car_Metadata_INSTANCE.getPropertyIndex("discontinued"),
                                                                                                                  defaultpkg.P51.LambdaExtractor519DC0D2200DB8C0CE09AEB4B0C3EA93.INSTANCE,
                                                                                                                  true),
                                                                                                 D.reactOn("discontinued")),
                                                                D.pattern(var_GENERATED_$pattern_Order$2$).expr("GENERATED_19C0B6B21686A8E04442698566247F3B",
                                                                                                                var_$car_1_sCoPe,
                                                                                                                defaultpkg.PEF.LambdaPredicateEF21722F0A1FCC0E95ACD94C1A6B2117.INSTANCE,
                                                                                                                D.betaIndexedBy(org.drools.model.codegen.execmodel.AccumulateTest.Car.class,
                                                                                                                                org.drools.model.Index.ConstraintType.EQUAL,
                                                                                                                                DomainClassesMetadataFCB815F80535171A856D047578567E4A.org_drools_model_codegen_execmodel_AccumulateTest_Order_Metadata_INSTANCE.getPropertyIndex("item"),
                                                                                                                                defaultpkg.P77.LambdaExtractor77565E1C1F219C6B34657CAF2D3078EB.INSTANCE,
                                                                                                                                defaultpkg.PBB.LambdaExtractorBBD0304C512D0A3ECA630EC377552F10.INSTANCE,
                                                                                                                                org.drools.model.codegen.execmodel.AccumulateTest.Car.class),
                                                                                                                D.reactOn("item")).bind(var_$price_1_sCoPe,
                                                                                                                                        defaultpkg.PC7.LambdaExtractorC7CBBC8CF2638092C1BDE953A419E4DE.INSTANCE,
                                                                                                                                        D.reactOn("price"))),
                                                          D.accFunction(RAccumulate3::new,
                                                                        var_$price_1_sCoPe).as(var_$total)),
                                             D.on(var_result,
                                                  var_$total).execute(defaultpkg.P5A.LambdaConsequence5A97B91F758DE5E929A604F8AF538731.INSTANCE));
        return rule;
    }
}
]]

Proposed Solution

The issue occurs due to non-deterministic behavior in the LambdaConsequence generated by MethodCallExpr and Consequence, where the execute() method’s parameters are passed in an inconsistent order. This causes Drools to fail to compile the generated Java file when running above test cases.

The root cause lies in the extractUsedDeclarations method in the Consequence object uses a Set to track and filter arguments, and this results the non-deterministic behavior.

To fix this, I replaced the Set with a LinkedHashSet in the extractUsedDeclarations method to ensure consistent ordering of declarations during rule compilation.

[everbrightw]

issue apache/incubator-kie-drools/issues/6118

[Derrick2000]

Hey, how were you able to trace/target the issue in the extractUsedDeclarations method from a Consequence object

[Derrick2000]

This PR will also fix the flaky test detected at org.drools.model.codegen.execmodel.GroupByTest.testDecomposedGroupByKeyAnd2AccumulatesInConsequence under module drools-model/drools-model-codegen

[Ethan-Chin]

This PR will also fix the flaky tests detected at org.drools.model.codegen.execmodel.AccumulateTest.testSemicolonMissingInInit
org.drools.model.codegen.execmodel.NamedConsequencesTest.testIfElseWithMvelAccessor
org.drools.model.codegen.execmodel.GroupByTest.testGroupBy2Vars

under module drools-model/drools-model-codegen

[SijieMei]

This PR will also fix flaky test detected at org.drools.model.codegen.execmodel.AccumulateTest.testAccumulateWithIndirectArgument

under module drools-model/drools-model-codegen

[mariofusco]

I've never seen any of those mentioned tests failing even once in my life. It is true that the values returned in that Set are iterated, but the order of iteration doesn't matter provided it is always stable during the same invocation. I believe those are all false negative generated by that NonIndex tool, and in this case replacing a HashSet with a LinkedHashSet for no valid reason implies and unnecessary performance cost.

[everbrightw]

Hi @mariofusco,

Thank you for taking the time to review and for your feedback.

I’d like to provide more context on my exploration of the codebase. Since this is my first time exploring this project, please bear with me if I am misunderstanding anything.

From my understanding, the usedDeclarationInRHS set is iterated over in two key places in the codebase:

1. It is used to construct the MethodCallExpr in onCall(Collection<String> usedArguments) by iterating at line over the HashSet

2. Then, during executeCall(), parameters are added to a NodeList by iterating over the same HashSet declarationUsedInRHS. The order of parameters can affect method signatures and variable bindings. Consequence.java#L291 which calls getBoxedParametersWithUnboxedAssignment

These two places iterate over the same HashSet, but due to the non-deterministic iteration order of HashSet, they can produce different orders during the same execution.

For instance, while running above test cases, I observed that the arguments passed to D.on(var_result, var_$eSet) could be in the order (result, $eSet) or ($eSet, result)., which leads to the above compilation error message.

Based on my understanding, iteration order in a HashSet is inconsistent in the same session itself , even within the same VM and even if the HashSet's contents aren't modified.

Thank you again for your time to read through this follow up.

[mariofusco]

These two places iterate over the same HashSet, but due to the non-deterministic iteration order of HashSet, they can produce different orders during the same execution.

This assumption isn't correct, or better isn't correctly formulated, and however it doesn't apply to the specific context where this issue has been reported. Let me explain.

It is true that the iteration order of an HashSet is not deterministic, but this doesn't mean that it randomically changes at each and every iteration. In particular for 2 iterations of the same unchanged HashSet instance in the same JVM execution it is guaranteed that the iteration order will be the same (no matter what it is) and this is enough for the use case that you analyzed.

I believe that this is a case that is not correctly covered by that NonIndex tool that you used, so maybe a problem that should be reported and fixed there.

For instance, while running above test cases, I observed that the arguments passed to D.on(var_result, var_$eSet) could be in the order (result, $eSet) or ($eSet, result)., which leads to the above compilation error message.

Based on my understanding, iteration order in a HashSet is inconsistent in the same session itself , even within the same VM and even if the HashSet's contents aren't modified.

As I wrote I've never seen this failure in any of the thousands of the test suite execution that I performed either on our CI environment or on my local machines. If you can reproduce it, this means that I'm wrong and I'd be glad to have a call so you could show me how to reproduce this issue.

[everbrightw]

Hi @mariofusco ,

Thanks for the guidance!

for 2 iterations of the same unchanged HashSet instance in the same JVM execution it is guaranteed that the iteration order will be the same

Thanks for pointing this out. My previous assumption was incorrect. For this case, the 'relatively deterministic' of the 2 iterations is sufficient.

I did some simple experiment TestingResearchIllinois/idoft#1273 (comment), and the reason that NonDex is making a 'false positive' for this case is because the two types of iterations being used: the Collection.forEach at code and Stream.forEach at code. Inherently, they are using different implementations: Stream API is using Splitereator and the 'normal' Collections forEach is using Iterator.

Right now NonDex is not shuffling Stream and caused the 'mismatch' between the two iterations during one execution in this particular case.

However, after reading some online posts and Javadoc for Stream API, I still have one unresolved question: should we expect Spliterator and Iterator to return elements in the same order during a single execution? (There was a issue reported here against the order of Stream.forEach but seems not exactly the same: https://bz.apache.org/netbeans/show_bug.cgi?id=257129)

Any insights on this would be greatly appreciated!

Sorry for the confusion to you earlier. I really appreciate the time you take in responding.

[mariofusco]

However, after reading some online posts and Javadoc for Stream API, I still have one unresolved question: should we expect Spliterator and Iterator to return elements in the same order during a single execution? (There was a issue reported here against the order of Stream.forEach but seems not exactly the same: https://bz.apache.org/netbeans/show_bug.cgi?id=257129)

That's a really good question (to which I wasn't actually thinking) and the short answer is that I don't know.

In general if a behaviour is not clearly stated in the contract of an API I assume that I cannot rely on it. However in this specific case I'd be very surprised if the Iterator and the single threaded Spliterator had a different iteration order: the goal of the Spliterator is recursively splitting a collection in chunks and the iterating any chunk independently, but if it doesn't perform any split (which is the case of the sequential execution) I don't see why the iteration order should be different by a normal Iterator.