Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a list of dates instead of a range #1743

Closed
AthiraHari77 opened this issue Jan 9, 2025 · 5 comments · Fixed by apache/incubator-kie-drools#6223
Closed

Return a list of dates instead of a range #1743

AthiraHari77 opened this issue Jan 9, 2025 · 5 comments · Fixed by apache/incubator-kie-drools#6223
Assignees
Labels
area:dmn Related to DMN area:engine Related to the runtime engines type:bug Something is behaving unexpectedly

Comments

@AthiraHari77
Copy link

AthiraHari77 commented Jan 9, 2025

There is a DMN TCK failure, when running with Drools:

[ERROR] 0084-feel-for-loops-test-01.decision_024 – Time elapsed: 0.001 s <<< ERROR!
java.lang.RuntimeException: ERROR: FAILURE: 'decision_024' expected='[1980-01-01, 1980-01-02, 1980-01-03]' but found='[[ 1980-01-01 .. 1980-01-03 ]]'

The test is located here (1). Based on the error, it looks like that Drools is returning a range instead of a list in this test case.

Reading the specs#10.3.2.14 For loop expression, it seems that a for iteration should always return a list

The result of the for loop expression is a list containing the result of the evaluation of the expression e for each
individual iteration in order

Options how to debug:

Replicating the test case directly in Drools repository, so it gets a proper stack trace pointing to the code path being executed, to be able to see the code areas, which may need a fix.

A specific unit test is required to test the modification in isolation.
In case of for expression, the class to test is ForExpressionNode, and there is already a unit-test class for it, ForExpressionNodeTest

In the past we used to add cases to "FEEL" test classes, but the problem with that approach are:

  1. the test involve bringing up all the FEEL compiler stack, so it can be influenced by lot of factors that are not relevant to the modification itself
  2. due to the above, debugging those tests is time-consuming and cumbersome while, at the end of the day, the actual code to verify is the method in the "Node" itself
  3. the above has a direct effect on the time required to deliver the feature and, on bigger scale, on the time spent in the CI.

To execute and eventually debug in isolation a single tck test, a small hack is required inside the TCK test suite, i.e. to add a "filter" in the DroolsTCKTest#addTestCasesFolders method

If the test case is mainly just a FEEL expression (FEEL is a language used to write expressions in DMN), the FEEL expression could be added to a test class, e.g. this one (2), so it can be run in isolation.

(1) https://github.com/dmn-tck/tck/tree/master/TestCases/compliance-level-3/0084-feel-for-loops

(2) https://github.com/apache/incubator-kie-drools/blob/053d70c8f9145706e771f90e8ce2dafa3899b59b/kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/runtime/FEELConditionsAndLoopsTest.java#L31

@yesamer yesamer added type:bug Something is behaving unexpectedly area:dmn Related to DMN area:engine Related to the runtime engines labels Jan 9, 2025
@yesamer yesamer assigned yesamer and unassigned yesamer Jan 9, 2025
@jomarko
Copy link

jomarko commented Jan 10, 2025

Thank you for reporting the issue. I am not expert in DMN arrays syntax. To me the current behavior makes a sense to some extent. However, if you say it is wrong, then probably more cases needs to be addressed. I mean decision_020_a for example:

for i in [@"1980-01-03T00:00:00"..@"1980-01-01T00:00:00"] return i

It currently also returns [[ 1980-01-03T00:00 .. 1980-01-01T00:00 ]]

@baldimir
Copy link

Hi @jomarko, the test mentioned in the description is just one picked for fixing. The actual fix may fix also other TCK failures, like the one you mentioned.

@yesamer
Copy link

yesamer commented Jan 10, 2025

Failures in 1156 Test suite could be related, it would be nice to double check.

@gitgabrio
Copy link

gitgabrio commented Jan 13, 2025

@baldimir @yesamer @AthiraHari77
Reading the comments, it seems we need to go back to specs and start from there, instead to try to fix "punctually" this tck test.

I also edit a bit the ticket description, mostly about testing approach

@gitgabrio
Copy link

Failures in 1156 Test suite could be related, it would be nice to double check.

I'm reading the 1156 test and, in theory, there should be no relation between the two.
But if, for some reason, we discover that modification in the ForExpression impact it, we need to double check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dmn Related to DMN area:engine Related to the runtime engines type:bug Something is behaving unexpectedly
Projects
None yet
5 participants