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

[JBPM-10078] Integration test for timer migration from 6.4 #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gmunozfe
Copy link
Member

Reproducer to validate fix provided by jbpm#2149

Copy link

@afalhambra afalhambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are few minor comments I posted, but one of them needs to be fixed.
Other than that, it's a very nice work!

<testResources>
<testResource>
<directory>src/test/resources</directory>
<filtering>false</filtering>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really needed, since maven filtering is set to false by default

</goals>
<configuration>
<outputDirectory>${project.build.directory}/drivers</outputDirectory>
<includeGroupIds>mysql</includeGroupIds>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really sure what driver version will be used? latest one?

<org.kie.samples.server>kie-server</org.kie.samples.server>
<org.jbpm.ejb.timer.local.cache>false</org.jbpm.ejb.timer.local.cache>
<org.jbpm.ejb.timer.tx>true</org.jbpm.ejb.timer.tx>
<org.kie.samples.image.postgresql>postgres:${image.postgresql.version}</org.kie.samples.image.postgresql>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we define mysql:tag image here instead of postgresql? and it should be pick up as a system property in the test class


import com.google.common.io.Files;

public class KieJarBuildHelper {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is not needed or used in this module

<logger name="org.testcontainers" level="info"/>
<logger name="com.github.dockerjava" level="info"/>

<root level="trace">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe debug or info is enough level, wdyt?

}

@AfterAll
public static void tearDown() throws Exception {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no checked exception is thrown here

}

@Test
void testEJBTimerWithRollback() throws InterruptedException, SQLException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, no need to declare these exceptions

ADD etc/jbpm-flow-7.69.0.Final.jar /WEB-INF/lib/jbpm-flow-7.69.0.Final.jar

RUN if [ -z "$NO_PATCH" ]; then \
rm $JBOSS_HOME/standalone/deployments/kie-server.war/WEB-INF/lib/jbpm-flow-7.69.0.Final.jar; \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue here. Since the image we are using it's based on quay.io/kiegroup/kie-server-showcase:7.70.0.Final the WEB-INF/lib/jbpm-flow-7.69.0.Final.jar will not exist.

Must be

Suggested change
rm $JBOSS_HOME/standalone/deployments/kie-server.war/WEB-INF/lib/jbpm-flow-7.69.0.Final.jar; \
rm $JBOSS_HOME/standalone/deployments/kie-server.war/WEB-INF/lib/jbpm-flow-7.70.0.Final.jar; \

Though if this is the case, maybe the patch is already included in 7.70.0.Final version?

Comment on lines +57 to +58
<org.jbpm.ejb.timer.local.cache>false</org.jbpm.ejb.timer.local.cache>
<org.jbpm.ejb.timer.tx>true</org.jbpm.ejb.timer.tx>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 2 properties belong to ejb timers, so not needed here

Comment on lines +133 to +141
List<TimerInstance> timers = processAdminClient.getTimerInstances(containerId, processInstanceId);

assertEquals(1, timers.size());

timers = processAdminClient.getTimerInstances(containerId, 6L);

logger.info("Retrieving timers for process 6; timers[6]: "+timers);

assertEquals(1, timers.size());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as already discussed, not sure about the migration process, since we are loading a mysql by using jbpm schemas version 6, and then we create a process instance through a kie-server 7.x? shouldn't we migrate the instance first? or am I missing anything here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants