[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… #5794

tkobayas · 2024-03-19T06:05:38Z

…ltiKieBaseTest

Issue:

[new-parser] Some rules do not fire in MultiKieBaseTest #5709

How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.

build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

How to retest this PR or trigger a specific build:

for pull request and downstream checks
- Push a new commit to the PR. An empty commit would be enough.
for a full downstream build
- for github actions job: add the label run_fdb
for Jenkins PR check only
- If you are an ASF committer for KIE podling, login to Jenkins (https://ci-builds.apache.org/job/KIE/job/drools/), go to the specific PR job, and click on Build Now button.

…ltiKieBaseTest

tkobayas · 2024-03-19T06:06:54Z

Fixed org.drools.model.codegen.execmodel.MultiKieBaseTest#testHelloWorldWithPackagesAnd2KieBases. Now all tests in MultiKieBaseTest are successful.

Before PR
drools-model-codegen

[ERROR] Tests run: 2433, Failures: 336, Errors: 0, Skipped: 9

After PR
drools-model-codegen

[ERROR] Tests run: 2433, Failures: 335, Errors: 0, Skipped: 9

tkobayas · 2024-03-19T06:08:38Z

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java

@@ -268,6 +268,7 @@ public RuleDescr visitRuledef(DRLParser.RuledefContext ctx) {
            ruleDescr.setConsequence(trimThen(getTokenTextPreservingWhitespace(ctx.rhs(), tokenStream))); // RHS is just a text
        }

+        populateStartEnd(ruleDescr, ctx);


The root cause was that RuleDescr.getStartCharacter() returned -1 (missed in DRLVisitorImpl), so the kbase was not updated (the error was logged). So fixed.

https://github.com/apache/incubator-kie-drools/blob/dev-new-parser/drools-compiler/src/main/java/org/drools/compiler/kie/util/ChangeSetBuilder.java#L247

Hi @tkobayas , thanks for the PR!
IIUC the comment, it means that a piece of code does not populate correctly a given object (ruleDescr), and your modification somehow "workaround" that.
I read the method, and TBH is somehow "brittle" by itself (I may give detailed explanation of this remark, if you want): would not be better to fix the root problem itself ? Eventually, I would be glad to help there

tkobayas · 2024-03-19T06:09:12Z

@yurloc @mariofusco @gitgabrio Please review, thanks!

gitgabrio

Thanks @tkobayas
but IIUC, the problem is elsewhere (please correct me if I'm wrong)

gitgabrio · 2024-03-19T08:16:59Z

drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java

@@ -268,6 +268,7 @@ public RuleDescr visitRuledef(DRLParser.RuledefContext ctx) {
            ruleDescr.setConsequence(trimThen(getTokenTextPreservingWhitespace(ctx.rhs(), tokenStream))); // RHS is just a text
        }

+        populateStartEnd(ruleDescr, ctx);


Hi @tkobayas , thanks for the PR!
IIUC the comment, it means that a piece of code does not populate correctly a given object (ruleDescr), and your modification somehow "workaround" that.
I read the method, and TBH is somehow "brittle" by itself (I may give detailed explanation of this remark, if you want): would not be better to fix the root problem itself ? Eventually, I would be glad to help there

gitgabrio · 2024-03-19T09:40:03Z

@mariofusco @baldimir @yesamer
Hi folks. This PR, accidentally, uncover two kinds of issues we are currently ignoring:

technical debt, that instead of being addressed is workarounded
increase of technical debt, polluting the code of such "workarounds"

IMO we have to to change the approach and the mindset, striving to improve the code whenever we have the chance: wdyt ?

tkobayas · 2024-03-19T10:11:16Z

@gitgabrio Thank you for the suggestion.

startCharacter and endCharacter are Descr's properties, so populating them is not a workaround, rather a right thing to do from the parser point of view.

Having said that,

This PR is okay as-is

and

It's great to refactor/improve the ChangeSetBuilder.diffDescrs method, so could you file it as a separate issue?

Does it make sense?

Thanks!

gitgabrio · 2024-03-19T10:55:56Z

@tkobayas
if

startCharacter and endCharacter are Descr's properties

why they are not already populated at that specific point, and the proposed modification has to populate them ?

populating them is not a workaround, rather a right thing to do from the parser point of view.

Yes, I understand that "from the parser point of view" this could be the right fix, but IIUC the problem is somewhere else (please correct me if I'm wrong, I'm simply inferring from your own comment) and we, as communtiy, have to manage all the codebase as a whole.
As usual, I may be wrong on all my remarks, and I can't wait to talk directly with you, to get to a better understanding.

tkobayas · 2024-03-21T03:16:46Z

if

startCharacter and endCharacter are Descr's properties

why they are not already populated at that specific point, and the proposed modification has to populate them ?

startCharacter and endCharacter should be populated when the RuleDescr is created, and the PR fix does it --- in the same method DRLVisitorImpl.visitRuledef.

Talking about the root cause "why we had this bug", I think of

We can improve the way to create Descr objects not to miss populating the properties in DRLVisitorImpl
Add more test cases

In this regard, I filed #5798

but IIUC the problem is somewhere else (please correct me if I'm wrong, I'm simply inferring from your own comment)

My comment:

The root cause was that RuleDescr.getStartCharacter() returned -1 (missed in DRLVisitorImpl), so the kbase was not updated (the error was logged). So fixed.

meant that the RuleDescr.startCharacter was -1 at the point. It had to be populated by DRLVisitorImpl beforehand. So I wanted to explain that the root cause was DRLVisitorImpl, not ChangeSetBuilder.diffDescrs.

I can't wait to talk directly with you, to get to a better understanding.

Sure, we can talk on meet or zoom.

gitgabrio

@tkobayas
Many thanks for the chat and the explanation: it clarifies things a lot!
With that in place, I see this PR is good to merge.
Thanks again!

tkobayas · 2024-03-21T08:21:09Z

@gitgabrio Thanks!

gitgabrio · 2024-03-21T08:29:51Z

@mariofusco @tkobayas @baldimir @yesamer
Follow up ticket

yurloc · 2024-03-21T12:18:56Z

Overall fixing progress: There were 407 failed tests.

…ltiKieBaseTest (apache#5794)

…ltiKieBaseTest (#5794)

…ltiKieBaseTest (apache#5794)

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu…

356ff24

…ltiKieBaseTest

tkobayas commented Mar 19, 2024

View reviewed changes

tkobayas requested review from mariofusco and gitgabrio March 19, 2024 06:09

gitgabrio requested changes Mar 19, 2024

View reviewed changes

mariofusco approved these changes Mar 19, 2024

View reviewed changes

tkobayas mentioned this pull request Mar 21, 2024

[new-parser] Ensure Descr properties #5798

Closed

gitgabrio approved these changes Mar 21, 2024

View reviewed changes

tkobayas merged commit 8bf5075 into apache:dev-new-parser Mar 21, 2024
0 of 3 checks passed

tkobayas mentioned this pull request Mar 21, 2024

[new-parser] Some rules do not fire in MultiKieBaseTest #5709

Closed

gitgabrio mentioned this pull request Mar 21, 2024

Fix ChangeSetBuilder.diffDescrs method apache/incubator-kie-issues#1026

Closed

tkobayas added a commit to tkobayas/drools that referenced this pull request Jun 11, 2024

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu…

8ec4736

…ltiKieBaseTest (apache#5794)

tkobayas added a commit to tkobayas/drools that referenced this pull request Oct 2, 2024

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu…

135559a

…ltiKieBaseTest (apache#5794)

tkobayas added a commit to tkobayas/drools that referenced this pull request Oct 11, 2024

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu…

f8ee445

…ltiKieBaseTest (apache#5794)

tkobayas added a commit that referenced this pull request Oct 11, 2024

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu…

0448475

…ltiKieBaseTest (#5794)

rgdoliveira pushed a commit to rgdoliveira/drools that referenced this pull request Oct 24, 2024

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu…

80f50b9

…ltiKieBaseTest (apache#5794)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… #5794

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… #5794

tkobayas commented Mar 19, 2024

tkobayas commented Mar 19, 2024 •

edited

Loading

tkobayas Mar 19, 2024

gitgabrio Mar 19, 2024

tkobayas commented Mar 19, 2024

gitgabrio left a comment

gitgabrio Mar 19, 2024

gitgabrio commented Mar 19, 2024

tkobayas commented Mar 19, 2024

gitgabrio commented Mar 19, 2024 •

edited

Loading

tkobayas commented Mar 21, 2024

gitgabrio left a comment •

edited

Loading

tkobayas commented Mar 21, 2024

gitgabrio commented Mar 21, 2024

yurloc commented Mar 21, 2024

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… #5794

[incubator-kie-drools#5709] [new-parser] Some rules do not fire in Mu… #5794

Conversation

tkobayas commented Mar 19, 2024

tkobayas commented Mar 19, 2024 • edited Loading

tkobayas Mar 19, 2024

Choose a reason for hiding this comment

gitgabrio Mar 19, 2024

Choose a reason for hiding this comment

tkobayas commented Mar 19, 2024

gitgabrio left a comment

Choose a reason for hiding this comment

gitgabrio Mar 19, 2024

Choose a reason for hiding this comment

gitgabrio commented Mar 19, 2024

tkobayas commented Mar 19, 2024

gitgabrio commented Mar 19, 2024 • edited Loading

tkobayas commented Mar 21, 2024

gitgabrio left a comment • edited Loading

Choose a reason for hiding this comment

tkobayas commented Mar 21, 2024

gitgabrio commented Mar 21, 2024

yurloc commented Mar 21, 2024

tkobayas commented Mar 19, 2024 •

edited

Loading

gitgabrio commented Mar 19, 2024 •

edited

Loading

gitgabrio left a comment •

edited

Loading