Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Proposal for tests for the NTP binding #1983

Closed
petarnv opened this issue Aug 9, 2016 · 6 comments
Closed

Proposal for tests for the NTP binding #1983

petarnv opened this issue Aug 9, 2016 · 6 comments

Comments

@petarnv
Copy link
Contributor

petarnv commented Aug 9, 2016

I would like to implement tests for the org.eclipse.smarthome.binding.ntp binding.
There are various test cases:

  1. We can put values in the configuration parameters such as hostname, timeZone, etc.
    and check if the state of the channels of the thing is updated with the right values.
  2. We can check that on handleCommand() and on channelLinked() the time is refreshed
    and because of that the state of the channels of the thing is updated.
  3. Some corner cases such as what happens if some of the configuration properties are not set,
    or were set, but with wrong values.

@marcelrv, as an initial contributor, do you think it is a good idea to implement tests for this binding?

@sjsf
Copy link
Contributor

sjsf commented Aug 9, 2016

as a general rule, IMHO it is always good to have tests for bindings, therefore any contribution is warmly welcome!
Just make sure that they don't rely on any real servers or even having internet access at all - everything has to run locally without relying on some external infrastructure.
Happy coding!

@marcelrv
Copy link
Contributor

marcelrv commented Aug 9, 2016

I agree, this would be a nice thing to have.
Also, as NTP binding is relatively simple this should not be a major task.

@petarnv, will you make a PR for this?

@petarnv
Copy link
Contributor Author

petarnv commented Aug 10, 2016

@marcelrv , yes I think to make a PR after I'm ready.

@petarnv
Copy link
Contributor Author

petarnv commented Aug 22, 2016

I have started working on this issue. With most of the tests cases everything is fine, but in the cases, where we have to test a time zone, there is a problem.
The string channel is updated with a string containing the network time, so the only thing that we can get from the item registry after the update is a date, formatted as a string.
With the Java 7 date time API we cannot parse this string to any object and keep the time zone. Parsing the string would mean loosing the time zone.
That leaves me with three options:

  1. Keep the Java 7 date time API and remove the tests for the time zone in the string channel;
  2. Use Joda Time library(http://www.joda.org/joda-time/index.html), which provides classes that would allow us to keep the time zone after parsing the string;
  3. Use Java 8 date time API, where an additional classes like ZonedDateTime are added, which would also allow us to keep the time zone after parsing the string. I'm not sure whether this may cause problem with the Maven build.

Any suggestions to which option should I choose?

@kaikreuzer
Copy link
Contributor

kaikreuzer commented Aug 22, 2016

  • We cannot do (3), since we have to be compatible with Java7.
  • Since we would like to move to Java8 soon, we should not do (2), since it would cause trouble when we upgrade (since then Java8 API would clearly be preferred over Joda Time).

-> I'd go for (1) for the moment.

@petarnv
Copy link
Contributor Author

petarnv commented Oct 4, 2016

I have made a PR for NTP Binding Tests: #2243

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

No branches or pull requests

4 participants