[incubator-kie-issues#1719] Enable onEntry/onExit scripts in embedded nodes #3815
Conversation
|
PR job Reproducerbuild-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u #3815 --skipParallelCheckout NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3815/1/display/redirect Test results:
Those are the test failures: org.jbpm.bpmn2.IntermediateEventTest.testMultiInstanceLoopCharacteristicsTaskWithOutputCmpCondSequentialExpected size: 1 but was: 0 in: [] |
There was a problem hiding this comment.
@martinweiler thank you for the PR. I am always amazed and inspired by the amount of work you are able to deliver.
I have few comments inline and one general question. Is the user task an embedded node? Asking as the only changed node in the PR is a human task. While I thought in BPMN
the embedded word is always used for embedded subprocesses. Sorry if the question is silly and just shows my missing knowledge.
| kruntime.getKogitoWorkItemManager().completeWorkItem(handler.getWorkItem().getStringId(), null); | ||
| assertProcessInstanceCompleted(processInstance); | ||
| assertThat(instance.status()).isEqualTo(ProcessInstance.STATE_ACTIVE); | ||
| instance.completeWorkItem(handler2.getWorkItem().getStringId(), null);//After this line, state of the process instance is error state instead of active |
There was a problem hiding this comment.
is the comment still true?
//After this line, state of the process instance is error state instead of active
I do not see any assertion that would contain value like ProcessInstance.STATE_ERROR.
There was a problem hiding this comment.
good catch, thanks! comment removed now
| <tns:onEntry-script> | ||
| <tns:script>System.out.println("Hello");context.setVariable("itemOut", "test");</tns:script> | ||
| <tns:onEntry-script scriptFormat="http://www.java.com/java"> | ||
| <tns:script>System.out.println("Hello");kcontext.setVariable("itemOut", "test");</tns:script> |
There was a problem hiding this comment.
As I sometimes need to read build logs I would like to share these ideas:
- is it worth to keep
System.out.println("Hello")? It adds one more line into logs, while there is still a java codekcontext.setVariable("itemOut", "test");that will be executed, so we will be still sure java in entry script works - maybe keep the log is fine, however
"Hello"is non unique. What do you think about"Hello testInclusiveSplitWithLoopInsideSubprocess"or anything that would be easier to greap or search in logs
There was a problem hiding this comment.
I think you have a valid point here. Those messages are just adding noise to the log files, and don't provide any real value. I have created apache/incubator-kie-issues#1726 now to look into this some more. This affects not only this particular bpmn file.
| @@ -837,12 +839,10 @@ public void beforeNodeTriggered(ProcessNodeTriggeredEvent event) { | |||
| } | |||
|
|
|||
| @Test | |||
| @Disabled("On Exit not supported, see https://issues.redhat.com/browse/KOGITO-2067") | |||
There was a problem hiding this comment.
There are 5 more tests disabled due to https://issues.redhat.com/browse/KOGITO-2067, shouldn't we allow also them?
There was a problem hiding this comment.
Yes, we should, and will. But some of them require additional fixes in the engine, so we will have to tackle them one by one.
|
|
||
| assertProcessInstanceActive(processInstance); | ||
| Process<InclusiveGatewayWithLoopInsideSubprocessModel> process = InclusiveGatewayWithLoopInsideSubprocessProcess.newProcess(app); |
There was a problem hiding this comment.
I am sorry, this is more my missing knowledge, where I can find the binding of
BPMN2-MultiInstanceLoopCharacteristicsTaskWithOutputCmpCondSequential.bpmn2 and InclusiveGatewayWithLoopInsideSubprocessProcess ? Or is the first mentioned edited as part of this PR but actually has noting with InclusiveGatewayWithLoopInsideSubprocessProcess?
There was a problem hiding this comment.
@jomarko thatInclusiveGatewayWithLoopInsideSubprocessModel is generated during the build, if you run a mvn clean package locally you'll find them in the project target folder under the generated-sources folder
martinweiler
force-pushed
the
incubator-kie-issues#1719
branch
from
907450e to
830ce44
Compare
|
@jomarko thanks for looking into this issue, and for your input. This is very much appreciated, as you bring in some new perspectives and raise valid points! Regarding your question on embedded nodes and user tasks, what is used in some of the tests here are multi-instance tasks (similar to MI subprocesses, but just in a single task). For the engine, this task is then embedded in a ForEachNode. I hope this helps to clarify. |
… nodes (apache#3815) * [incubator-kie-issues#1719] Enable onEntry/onExit scripts in embedded nodes * Fix process to use java syntax for the onEntry script * Remove outdated comment
Closes apache/incubator-kie-issues#1719