Complements support for FEEL Parser isDynamicResolution #1208

tarilabs · 2017-04-13T16:23:21Z

No description provided.

tarilabs

I am getting for the test

FlightRebookingTest.testSolutionAlternate()

18:16:29.695 [main] WARN  org.kie.dmn.core.util.MsgUtil.logMessage:68 - Missing expression for Decision Node 'Rebooked Passengers'

which I'll need to investigate with more time, but as mentioned @etirelli I preferred to anticipate this PR for any further comments please?

tarilabs · 2017-04-13T16:23:52Z

kie-dmn/kie-dmn-core/src/test/resources/org/kie/dmn/core/0019-flight-rebooking-alternative.dmn

@@ -198,7 +198,7 @@
 				<contextEntry>
 					<variable name="Best Alternate Flight" typeRef="kie:tFlight"/>
 					<literalExpression>
-						<text>Flight List[ From = Original Flight.From and 
+						<text>Flights[ From = Original Flight.From and 


This was pointing to a InputData instead of the BKM parameter

Thanks for fixing it.

tarilabs · 2017-04-13T16:24:14Z

kie-dmn/kie-dmn-core/src/test/resources/org/kie/dmn/core/0019-flight-rebooking-alternative.dmn

@@ -95,7 +95,7 @@
 						<text> status priority( Passenger1.Status ) &lt; status priority( Passenger2.Status ) or
 							   ( status priority( Passenger1.Status ) = status priority( Passenger2.Status ) and 
 							     Passenger1.Miles > Passenger2.Miles ) 
-							   )</text>
+						</text>


There was a superfluous additional end parenthesis )

etirelli · 2017-04-13T18:06:04Z

kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/parser/feel11/ParserHelper.java

@@ -180,7 +180,7 @@ public void startVariable(Token t) {
    }

    public boolean followUp(Token t, boolean isPredict) {
-        boolean follow = this.currentScope.followUp( t.getText(), isPredict );
+        boolean follow = ( isDynamicResolution() && FEELParser.isVariableNameValid( t.getText() ) ) || this.currentScope.followUp( t.getText(), isPredict );


Unfortunately you will have to create a new method: FEELParser.isVariablePartNameValid(). For instance, a variable can contain numbers, but cannot start with a number. So I am afraid what you have here will fail for a name like: "My Variable 1", because the method will be called for each token: "Variable" (succeeds), "1" (fails).

etirelli · 2017-04-13T18:06:40Z

Looks good, just see my comment on the variable name check.

tarilabs · 2017-04-14T07:01:28Z

Internal CI error:

16:41:54 org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test (default-test) on project kie-dmn-core: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.18.1:test failed: java.lang.NullPointerException: Null output stream
16:41:54 	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:224)
...
16:41:54 Caused by: java.lang.NullPointerException: Null output stream
16:41:54 	at java.io.PrintStream.requireNonNull(PrintStream.java:79)
16:41:54 	at java.io.PrintStream.<init>(PrintStream.java:151)
16:41:54 	at java.io.PrintStream.<init>(PrintStream.java:135)
16:41:54 	at org.apache.tools.ant.taskdefs.Redirector.handleOutput(Redirector.java:791)
16:41:54 	at org.apache.tools.ant.taskdefs.Java.handleOutput(Java.java:670)
16:41:54 	at org.apache.tools.ant.UnknownElement.handleOutput(UnknownElement.java:214)
16:41:54 	at org.apache.tools.ant.Project.demuxOutput(Project.java:1268)
16:41:54 	at org.apache.tools.ant.DemuxOutputStream.processBuffer(DemuxOutputStream.java:173)
16:41:54 	at org.apache.tools.ant.DemuxOutputStream.write(DemuxOutputStream.java:148)
16:41:54 	at org.apache.tools.ant.DemuxOutputStream.write(DemuxOutputStream.java:242)
16:41:54 	at java.io.PrintStream.write(PrintStream.java:480)
16:41:54 	at org.apache.maven.plugin.surefire.report.DirectConsoleOutput.writeTestOutput(DirectConsoleOutput.java:50)
16:41:54 	at org.apache.maven.plugin.surefire.report.TestSetRunListener.writeTestOutput(TestSetRunListener.java:108)
16:41:54 	at org.apache.maven.plugin.surefire.booterclient.output.ForkClient.consumeLine(ForkClient.java:145)
16:41:54 	at org.apache.maven.plugin.surefire.booterclient.output.ThreadedStreamConsumer$Pumper.run(ThreadedStreamConsumer.java:67)
16:41:54 	... 1 more

Jenkins test again

…e tokens check

tarilabs · 2017-04-14T12:37:28Z

unrelated test error

Test Result (1 failure / +1)
org.drools.compiler.integrationtests.ParallelEvaluationTest.testFireUntilHaltWithExpiration

Jenkins test again 🎱

tarilabs · 2017-04-14T13:08:16Z

Internal CI error:

13:00:36 [INFO] ------------------------------------------------------------------------
13:00:36 [INFO] BUILD SUCCESS
13:00:36 [INFO] ------------------------------------------------------------------------
13:00:36 [INFO] Total time: 16:54 min (Wall Clock)
13:00:36 [INFO] Finished at: 2017-04-14T09:00:35-04:00
13:00:37 [INFO] Final Memory: 474M/1937M
13:00:37 [INFO] ------------------------------------------------------------------------
13:00:37 [WARNING] The requested profile "wildfly10" could not be activated because it does not exist.
13:00:37 No emails were triggered.
13:00:37 Recording test results
13:00:45 Build step 'Publish JUnit test result report' changed build result to UNSTABLE
13:00:45 [FINDBUGS] Collecting findbugs analysis files...

Jenkins test again 🎱

psiroky · 2017-04-14T13:10:51Z

@tarilabs I don't think the last failure is "internal CI error". One of the tests (randomly) failed.

tarilabs · 2017-04-14T13:14:37Z

@psiroky but the build status is SUCCESS, if a failing test was failing it would have mentioned FAILURE ?

tarilabs · 2017-04-14T13:34:30Z

Internal CI error (SSH channel is closed)

13:13:23 GitHub pull request #1208 of commit 4495c0f1d58be036c8bb26a3c320d78b4a9f767b, no merge conflicts.
13:13:24 Setting status of 4495c0f1d58be036c8bb26a3c320d78b4a9f767b to PENDING with url https://kie-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/drools-pullrequests/1031/ and message: 'Build started sha1 is merged.'
13:13:24 Using context: Linux
13:13:24 [EnvInject] - Loading node environment variables.
13:13:24 Building remotely on rhel7-medium-3402 (ff linux mem4g gui-testing rhel7) in workspace /home/jenkins/workspace/drools-pullrequests
13:13:24 [Process cleanup] Running.
13:13:24 DEBUG: ' 6713  6346 sshd: jenkins@notty'
13:13:24 DEBUG: ' 7323  6713 bash -c cd "/home/jenkins" && java -Xmx700m -jar slave.jar'
13:13:24 DEBUG: ' 7334  7323 java -Xmx700m -jar slave.jar'
13:13:24 DEBUG: ' 8300  7334 ps -u jenkins -o pid,ppid,args'
13:13:24 Killing Process PID = 8300, PPID = 7334, ARGS = ps -u jenkins -o pid,ppid,args
13:29:10 FATAL: java.io.IOException: SSH channel is closed
13:29:10 hudson.remoting.RequestAbortedException: java.io.IOException: SSH channel is closed

Jenkins test again 🎱

psiroky · 2017-04-14T13:44:20Z

@tarilabs no, the Maven result will always be SUCCESS in case some of tests fail (see the previous builds for example). The reason for that is that we need to distinguish between three results:

build error - this means the job itself failed to finish. e.g. Maven itself failed during compilation. The build is RED
test failures - the build finished, but there some test failed. The build is YELLOW.
successful build - the build is GREEN

If the Maven build would fail for the test failures, it would be harder to distinguish between 1) and 2) as the build would always be RED.

Note: there is of course specific configuration for that (-Dmaven.test.failure.ignore=true). By default the build would fail (RED).

tarilabs · 2017-04-14T13:47:53Z

@psiroky I'm a bit confused because normally in scenario n2 I get the list of the failing test in the Jenkins "landing page" of the build.

psiroky · 2017-04-14T13:50:27Z

@tarilabs correct. Looking at https://kie-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/drools-pullrequests/1030/ (which is the build in question), the failures are there.

tarilabs · 2017-04-14T13:55:17Z

@psiroky build 1030 cannot be the build in question because is a build for PR #1209 which is not this PR but another one :)

tarilabs · 2017-04-14T13:57:11Z

the build mentioned in https://github.com/kiegroup/drools/pull/1208#issuecomment-294150716
is https://kie-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/drools-pullrequests/1029/
but now indeed I can see the test summary on the build landing page

tarilabs · 2017-04-14T14:01:23Z

It may be possible I have not noticed the test failure (1) because with the same test failure of the previous build in the build sequence on CI, only "new failing test" are reported in the summary - if the landing page has this behavior for that section.

tarilabs · 2017-04-14T14:14:35Z

kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/parser/feel11/FEELParser.java

+    /**
+     * Either namePart is a string of digits, or it must be a valid name itself 
+     */
+    public static boolean isVariableNamePartValid( String namePart ) {


highlighting main change since last review is this^ method

@tarilabs unfortunately, it is not only digits that are valid... all the additional characters listed in the spec would also need to be supported (see https://github.com/kiegroup/drools/blob/master/kie-dmn/kie-dmn-feel/src/main/antlr4/org/kie/dmn/feel/parser/feel11/FEEL_1_1.g4#L191 ). But since this is a temporary solution until we have proper type support, lets go with this as is and focus on implementing type support.

@etirelli I tried that with a more general approach as captured in commit 4eca10e but I believe it was generating more troubles than benefit... (in fact it was breaking the test for the flight rebooking)

etirelli · 2017-04-15T13:01:56Z

Merging, as all tests passed.

…apache#1208) * Demonstrate isDynamicResolution limitation * Resolve isDynamicResolution limitation for variable name tokens check * Solution 2 to resolve isDynamicResolution limitation for variable name tokens check (cherry picked from commit 913ed2e)

…#1208) * Demonstrate isDynamicResolution limitation * Resolve isDynamicResolution limitation for variable name tokens check * Solution 2 to resolve isDynamicResolution limitation for variable name tokens check (cherry picked from commit 913ed2e)

…che#1208)

tarilabs added 4 commits April 12, 2017 20:01

WIP

aef432f

WIP

53ab33f

.

e19a82d

.

522fa61

tarilabs commented Apr 13, 2017

View reviewed changes

etirelli reviewed Apr 13, 2017

View reviewed changes

tarilabs added 3 commits April 14, 2017 11:41

Demonstrate isDynamicResolution limitation

195f532

Resolve isDynamicResolution limitation for variable name tokens check

4eca10e

Solution 2 to resolve isDynamicResolution limitation for variable nam…

4495c0f

…e tokens check

tarilabs commented Apr 14, 2017

View reviewed changes

etirelli approved these changes Apr 15, 2017

View reviewed changes

etirelli merged commit 913ed2e into apache:master Apr 15, 2017

Rikkola pushed a commit to Rikkola/drools that referenced this pull request Jan 27, 2020

[DROOLS-4414] Updated to comply with twin modification on drools (apa…

fa87d2e

…che#1208)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complements support for FEEL Parser isDynamicResolution #1208

Complements support for FEEL Parser isDynamicResolution #1208

tarilabs commented Apr 13, 2017

tarilabs left a comment

tarilabs Apr 13, 2017

etirelli Apr 13, 2017

tarilabs Apr 13, 2017

etirelli Apr 13, 2017

etirelli Apr 13, 2017

etirelli commented Apr 13, 2017

tarilabs commented Apr 14, 2017 •

edited

Loading

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

psiroky commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

psiroky commented Apr 14, 2017 •

edited

Loading

tarilabs commented Apr 14, 2017

psiroky commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs Apr 14, 2017

etirelli Apr 15, 2017

tarilabs Apr 15, 2017

etirelli commented Apr 15, 2017

Complements support for FEEL Parser isDynamicResolution #1208

Complements support for FEEL Parser isDynamicResolution #1208

Conversation

tarilabs commented Apr 13, 2017

tarilabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etirelli commented Apr 13, 2017

tarilabs commented Apr 14, 2017 • edited Loading

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

psiroky commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

psiroky commented Apr 14, 2017 • edited Loading

tarilabs commented Apr 14, 2017

psiroky commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

tarilabs commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etirelli commented Apr 15, 2017

tarilabs commented Apr 14, 2017 •

edited

Loading

psiroky commented Apr 14, 2017 •

edited

Loading