timer2

[tom.baeyens]

pascal,

your timer updates are committed

the timer job-executor-notification on timer repeat is a real fix ! great.

but the test is still commented. just like the other tests. it would be great if you could update them. same comments as for the other tests apply

when do you think these updates can be done ?

[tom.baeyens]

pascal, can you explain the purpose behind the timer2 tests ?

[pascal.verdage]

Hi,

Thanks for the commit. I will work on updating the tests and should come up with a first version probably today.

Due to the refactoring, I created a new package but I will move the tests in timer package. I forgot to mention it when I mailed you the patch.
The test provides functional tests and tool methods in order to make the tests more readable. Some of these methods will move in a TimerTestCase class when I provide some integration tests, as many timers with the same deadline for example.

Regards,
Pascal

[pascal.verdage]

Hi,

I updated Timer tests and added another test with simultaneous timers. They are located in package org.jbpm.env.timer. But I think they should be split in:
1) basic timer tests using the TestTimerSession you created
2) other tests in JobExecutorTimerSessionTest
I will do it tomorrow.
You will see that I followed your advice regarding inheritance in these tests.

BTW, I found two errors in the DbTest suite while adding mine. They can be reproduced by duplicating the test JobExecutorTests. One of them can be fixed by inheriting DbTestCase (test fails because db is not cleaned) but I did not fix it in this commit. I didn't look closely at the other one.

Regards,
Pascal

[tom.baeyens]

"pascal.verdage" wrote:
But I think they should be split in:
1) basic timer tests using the TestTimerSession you created
2) other tests in JobExecutorTimerSessionTest

can you explain in detail what you mean here ?

we need to get this right. better to explain exactly the test scenarios you envision before spending the time to code them.

[pascal.verdage]

Hi,

currently the provided tests test both Timer and JobExecutorTimerSession. I think it would be better to separate these aspects. It would also allow to have similar tests for other TimerSession implementations without many code duplication. Finally, I think the tests would be more readable.

In the package org.jbpm.pvm.job, I would put tests for timers which don't depend on a 'production' TimerSession implementation nor JobExecutor:

public class TestTimerSession {
 public void schedule(Timer timer) {...}
 public void cancel(Timer timer) {...}

 /** execute the first timer (ordered by dueDate) */
 public void executeFirstTimer() {...}

 /** return the first timer (ordered by dueDate) */
 public Timer getFirstTimer() {...}

 /** return the number of scheduled timers */
 public int getNbTimer()
}

The tests would be:
timer with no activity/signal/event: set its date (setDueDate and setDueDateDescription methods), possibly a repeat, and check afterwards if it is still scheduled
use no repeat and set signal and/or activity. We need a special class to check the result:

public class TestActivityInstance extends ActivityInstance {
 public void signal(String signalName, Map<String, Object> parameters) {...}
}

use no repeat and set event and/or activity. We might a special executionImpl class to check the result (or maybe a listener?)


In the package org.jbpm.jobexecutor, I would put JobExecutorTimerSession tests:
validation of the timer when scheduling
cancel (before end of transaction, before execution, after a few executions)
schedule (in the past, in the future, with repeat)
execuetion of many timers on the same date
A special Timer class will ease the tests:

public class TestTimer extends Timer {
 public Boolean execute(Environment environment) throws Exception {...}
}

Only the second tests would be persisted.

Regards,
Pascal

[tom.baeyens]

Pascal, can you first check if the tests run on your side ? The updates broke the tests on my side, i think.

[tom.baeyens]

there is also a test that keeps on running.

please make sure that for tests with multiple threads, you make sure that you install a timer task that interrupts the thread when something goes not as expected.

currently there is a test that keeps running and blocks the rest of the test suite to be executed.

check out how i have done it in the job executor test and use that as a basis

[tom.baeyens]

for the timer test, the test environment needs to be set up and an environment needs to be passed into the timer execute.

adding if statements in the impl to check for environment is not the right testing strategy.

checking the working of the timer class, you should provide it with an environment from which it will obtain the test session from there

adding the null pointer checks was not good.

overall, i think that the timer test as it is now don't make much sense. the execute method of the timer fits into the whole of the job executor. i don't think it should be checked in isolation

[tom.baeyens]

overall, i think i prefer that we test the timers against the api in one of the settings that we envision.

first we need to have a better understanding of what exactly the API is to create timers. i see two forms in the API to create timers

1) creating timers declaratively on process definitions and nodes and then executing an execution.

2) creating a definition, an execution and creating the timers programmatically through the execution.createTimer methods

then we need to verify that these timer creations get properly executed in the environments that we support. one of the environments to execute the timers is the job executor in a standard java environment. that is the first target.

this sketches a bit where i want to go with the timer testing.

the way that currently the timer is tested is a mix of implementation details and api. i believe that the exact behaviour of the timer methods should be tested in the different environments and with the api's as outlined above.

[pascal.verdage]

Hi Tom,

Pascal, can you first check if the tests run on your side ?

I did it (about 40 times to check that 100 simultaneous timers with 30 jobExecutor threads running was supported) and there was no problem. I just ran the AllTests suite about ten times on a fresh checkout and I encountered no problem.
Could it be a different JDK (I'm running sun 5.0 JDK)?

please make sure that for tests with multiple threads, you make sure that you install a timer task that interrupts the thread when something goes not as expected.

Good idea! I forgot to ensure that.

adding if statements in the impl to check for environment is not the right testing strategy.

I agree. Still, I think it is better to check that the environment exists. Maybe it could be done in CommandService implementations because if the interceptor is missing there would be a NullPointerException.

Regarding your last post, I intended to add such tests but I still have some problems with the ProcessDefinitionFactory so this isn't done yet. I agree there should be more integration tests.
However, I thought it would be good to have unit tests to assert the behaviour of the Timer and TimerSession implementations. This is the reason for the tests I provided, event though these tests misuse timers.

Regards,
Pascal

[tom.baeyens]

"pascal.verdage" wrote:
Hi Tom,

Pascal, can you first check if the tests run on your side ?

I did it (about 40 times to check that 100 simultaneous timers with 30 jobExecutor threads running was supported) and there was no problem. I just ran the AllTests suite about ten times on a fresh checkout and I encountered no problem.
Could it be a different JDK (I'm running sun 5.0 JDK)?

my bad. i made a local update. probably svn was ok.

"pascal.verdage" wrote:

adding if statements in the impl to check for environment is not the right testing strategy.

I agree. Still, I think it is better to check that the environment exists. Maybe it could be done in CommandService implementations because if the interceptor is missing there would be a NullPointerException.

an exception is in that case better then ignoring it.

"pascal.verdage" wrote:
Regarding your last post, I intended to add such tests but I still have some problems with the ProcessDefinitionFactory so this isn't done yet. I agree there should be more integration tests.
However, I thought it would be good to have unit tests to assert the behaviour of the Timer and TimerSession implementations. This is the reason for the tests I provided, event though these tests misuse timers.

i'm currently thinking about making the separation between api and internal classes more clear. that's related to this discussion. if the api is more clear, then finding the right tests will become clearer as well.

[pascal.verdage]

an exception is in that case better then ignoring it.

I agree, but wouldn't it be better to have an explicit PvmException("no environment") than NullPointer?

Pascal, can you first check if the tests run on your side ?

I ran the tests suite with Postgres and there are indeed errors as you specified it.
My apologies.
However, after a quick look at these failed tests, they do not seem to be related to timers.

Regards,
Pascal

[tom.baeyens]

"pascal.verdage" wrote:

an exception is in that case better then ignoring it.

I agree, but wouldn't it be better to have an explicit PvmException("no environment") than NullPointer?

Pascal, can you first check if the tests run on your side ?

I ran the tests suite with Postgres and there are indeed errors as you specified it.
My apologies.
However, after a quick look at these failed tests, they do not seem to be related to timers.

explicit PvmException is better. i always use the message "variableName is null"

keep me posted on the progress with the postgres errors. if you check in problems with some db is not a real problem. the AllTests are the criterium. if that succeeds after an update you can commit.