You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "everbrightw (via GitHub)" <gi...@apache.org> on 2024/10/09 22:06:07 UTC
[PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
everbrightw opened a new pull request, #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119
### 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:
<details>
<summary>Click to view</summary>
```
[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;
}
}
]]
```
</details>
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "Derrick2000 (via GitHub)" <gi...@apache.org>.
Derrick2000 commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2403988626
Hey, was wondering how you were able to target `extractUsedDeclarations` at [Consequence.java](https://github.com/apache/incubator-kie-drools/pull/6119/files#diff-13e49c05a33c7f4af1218416bb955343fd55459c7910500e92ae6f78922a792b) to make changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "Derrick2000 (via GitHub)" <gi...@apache.org>.
Derrick2000 commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2406487148
This PR will also fix the flaky test detected at `org.drools.model.codegen.execmodel.GroupByTest.testDecomposedGroupByKeyAnd2AccumulatesInConsequence` under module `drools-model/drools-model-codegen`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "Ethan-Chin (via GitHub)" <gi...@apache.org>.
Ethan-Chin commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2406497432
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "everbrightw (via GitHub)" <gi...@apache.org>.
everbrightw commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2411467310
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](https://github.com/apache/incubator-kie-drools/blob/9711a149672bc6721288e9ec16374a5b42d4a97e/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/Consequence.java#L305) 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](https://github.com/apache/incubator-kie-drools/blob/9711a149672bc6721288e9ec16374a5b42d4a97e/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/Consequence.java#L291) which calls [getBoxedParametersWithUnboxedAssignment](https://github.com/apache/incubator-kie-drools/blob/9711a149672bc6721288e9ec16374a5b42d4a97e/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/BoxedParameters.java#L45)
And from my exploration while running the test case, the order that are being passed to `D.on(var_result,var_$eSet)` could be both `(result, $eSet)` and `($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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "SijieMei (via GitHub)" <gi...@apache.org>.
SijieMei commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2409212484
This PR will also fix flaky test detected at org.drools.model.codegen.execmodel.AccumulateTest.testAccumulateWithIndirectArgument
under module drools-model/drools-model-codegen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco closed pull request #6119: [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest
URL: https://github.com/apache/incubator-kie-drools/pull/6119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "everbrightw (via GitHub)" <gi...@apache.org>.
everbrightw commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2419914068
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.
### For more context:
I did some simple experiment https://github.com/TestingResearchIllinois/idoft/pull/1273#issuecomment-2415929519, 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](https://github.com/apache/incubator-kie-drools/blob/9711a149672bc6721288e9ec16374a5b42d4a97e/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/BoxedParameters.java#L47) and Stream.forEach at [code](https://github.com/apache/incubator-kie-drools/blob/9711a149672bc6721288e9ec16374a5b42d4a97e/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/Consequence.java#L305). 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2410259686
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "Derrick2000 (via GitHub)" <gi...@apache.org>.
Derrick2000 commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2404047083
Hey, how were you able to trace the issue in the `extractUsedDeclarations` method from a `Consequence` object
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccumulateTest [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2420108837
> 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "everbrightw (via GitHub)" <gi...@apache.org>.
everbrightw commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2403514881
issue [apache/incubator-kie-drools/issues/6118](https://github.com/apache/incubator-kie-drools/issues/6118)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#6118] Fix flaky tests in AccmulateTest [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco commented on PR #6119:
URL: https://github.com/apache/incubator-kie-drools/pull/6119#issuecomment-2411601021
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org