13 Replies Latest reply on May 22, 2008 10:34 AM by Tom Baeyens

    timer2

    Tom Baeyens Master

      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 ?

        • 1. Re: timer2
          Tom Baeyens Master

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

          • 2. Re: timer2
            Pascal Verdage Newbie

            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

            • 3. Re: timer2
              Pascal Verdage Newbie

              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

              • 4. Re: timer2
                Tom Baeyens Master

                 

                "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.

                • 5. Re: timer2
                  Pascal Verdage Newbie

                  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

                  • 6. Re: timer2
                    Tom Baeyens Master

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

                    • 7. Re: timer2
                      Tom Baeyens Master

                      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

                      • 8. Re: timer2
                        Tom Baeyens Master

                        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

                        • 9. Re: timer2
                          Tom Baeyens Master

                          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.

                          • 10. Re: timer2
                            Pascal Verdage Newbie

                            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

                            • 11. Re: timer2
                              Tom Baeyens Master

                               

                              "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.

                              • 12. Re: timer2
                                Pascal Verdage Newbie

                                 

                                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

                                • 13. Re: timer2
                                  Tom Baeyens Master

                                   

                                  "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.