Eclipse Community Forums: Eclipse SmartHome

Help

Home

Home » Archived » Eclipse SmartHome » Timer reschedule()(Contributor advice needed)

Show: Today's Messages :: Show Polls :: Message Navigator

Timer reschedule() [message #1797772]

Tue, 06 November 2018 09:55

Jon Evans

Messages: 3
Registered: July 2009

Junior Member

Hi

I have reported a bug with the implementation of Timer#reschedule().

The JavaDoc for Timer#reschedule() says that you can call reschedule() at any time, even on a terminated timer ("This can also be called after a timer has terminated, which will result in another execution of the same code."). In practice, you can only call reschedule() either before the timer fires, or from the body of the timer while it is executing. You can't call it after it has executed (i.e. after hasTerminated() returns true).

I would like to fix this.

The first thing I've found is that there are currently no unit tests for the org.eclipse.smarthome.model.script.actions.ScriptExecution class, which is the class containing the createTimer method used in rules files. So, I've written a unit test for the current behaviour. I then went further and fixed the issue, and added a test for that as well. In order to fix the issue I had to modify the TimerImpl class so that it could hold onto the JobDataMap in case it had to schedule a new job.

I'm looking for advice about contributing this, as the contributor guidelines say to ask here first.

* My commit adding the unit test
* My commit fixing the bug and adding a test for it

So the advice I'm soliciting is:

* Does the approach look good?
* I didn't find a good way to test the interaction with the Quartz scheduler, so the test creates a job which fires in 10 milliseconds, then calls Thread.sleep(30) to wait for it to run. I couldn't find a better way but I'm open to ideas.
* I can run my individual unit test from the Eclipse IDE and it passes. mvn test fails due to other tests not passing.
* Is there anything else I've missed?
* Does anyone know how to set up Eclipse IDE so that it will pull down source code for dependencies? It seems to be disabled at the moment (set up via the official setup guide) because it doesn't consider any of the projects to be Maven projects.
* Do I need to fix up anything before creating Pull Requests, or should I just create them so people can comment on GitHub?

Report message to a moderator

Re: Timer reschedule() [message #1797830 is a reply to message #1797772]

Wed, 07 November 2018 07:47

Henning Treu

Messages: 44
Registered: April 2017

Member

Hi Jon,

thanks a lot for taking this on!

You had me at
Quote:

So, I've written a unit test for the current behaviour.

Awesome to add tests first!

Please just follow the contribution guidelines (like signing the ECA, adding singed-off-by footer to the commit) and create a pull request on github. Other contributors and committers will start the review process there and give comments on your code. If necessary we refine your PR together and finally merge it.

I'm happy to find your contribution on github. See you there.

Cheers
Henning

Report message to a moderator