Errors in PR #57

[jimtng]

There are some errors merged from #57 time_of_day.rb and dsl.rb I think, still have merge artifacts.

[jimtng]

Perhaps a good idea to have some sort of auto check / CI to at least check for ruby syntax errors on PRs, similar to what they do on openhab repo.

[boc-tothefuture]

Oof.

Issue #2 tracks the need to setup CI/CD.

[boc-tothefuture]

@jimtng I fixed the merge conflicts but the TimeOfDay tests aren't passing.. Can you take a look?

[jimtng]

regression error. You added in check_log

def check_log(entry)
  if File.foreach(openhab_log).any? { |line| line.include?('Error during evaluation of script') }
    raise 'Error in script'
  end

The cucumber tests failed in the 'should not' cases. When the wrong time format is being parsed by TimeOfDay.parse(), an exception is raised, causing Error during evaluation of script which your check_log picked up which in turn failed the "when I deploy the rule".

How should we address this? Should the TimeOfDay.parse() handle the exception and issue logger.error() instead?

[boc-tothefuture]

I did see that, but I am failing parsing anything with am/pm in it.. not just the ones that shouldn't log.

Failing Scenarios:
cucumber features/time_of_day.feature:36 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:38 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:39 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:40 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:41 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:42 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:43 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:44 # Scenario Outline: Parse strings into a TimeOfDay object

I think the right way to handle it is to catch the java exception (haven't done that yet in JRuby) and then raising an ArgumentError.

Then in the test we should wrap the check in a

begin
rescue

and log that we caught the exception.

[jimtng]

My test output:

Failing Scenarios:
cucumber features/time_of_day.feature:42 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:43 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:44 # Scenario Outline: Parse strings into a TimeOfDay object

13 scenarios (3 failed, 10 passed)
52 steps (3 failed, 3 skipped, 46 passed)

[boc-tothefuture]

You are on the main branch?

[jimtng]

You are on the main branch?

Yes Commit 6f051ba35dafa3da06c9091c9314a79c757baae4

[jimtng]

Wrapping the java exception and re-raising it as ArgumentError, like this?

          def self.parse(string)
            format = /(am|pm)$/i.match?(string) ? 'h[:mm[:ss]][ ]a' : 'H[:mm[:ss]]'
            begin
              local_time = LocalTime.parse(string.downcase, DateTimeFormatter.ofPattern(format))
            rescue java.time.format.DateTimeParseException => e
              raise ArgumentError, e.message
            end
            TimeOfDay.new(h: local_time.hour, m: local_time.minute, s: local_time.second)
          end

[boc-tothefuture]

TimeOfDay.new should go in the begin block.

[jimtng]

TimeOfDay.new should go in the begin block.

Why?

[boc-tothefuture]

Actually, you might not need the begin block at all.. because it is in a method. I think its just

 def self.parse(string)
            format = /(am|pm)$/i.match?(string) ? 'h[:mm[:ss]][ ]a' : 'H[:mm[:ss]]'
            local_time = LocalTime.parse(string.downcase, DateTimeFormatter.ofPattern(format))
            TimeOfDay.new(h: local_time.hour, m: local_time.minute, s: local_time.second)
            rescue java.time.format.DateTimeParseException => e
               raise ArgumentError, e.message
          end

http://seejohncode.com/2012/04/17/short-tip-rescuing-a-method-in-ruby/

[jimtng]

Actually, you might not need the begin block at all.. because it is in a method.

Then no question about where TimeOfDay.new should be. I love the short syntax, nice and sweet.

[jimtng]

I made a quick PR for that. Could you fix up the cucumber side of things, @boc-tothefuture ?

[boc-tothefuture]

I will, but it's still failing on what I think are correct use cases:

ArgumentError: Text '1pm' could not be parsed at index 1
   parse at /Users/boc@us.ibm.com/personal/src/openhab-scripting/tmp/openhab/conf/automation/lib/ruby/lib/openhab/core/dsl/time_of_day.rb:59
  <main> at <script>:7

[jimtng]

Curious. Could you paste your time_of_day.rb here please?

My test results here:

  Scenario Outline: Parse strings into a TimeOfDay object               # features/time_of_day.feature:21
    Given a rule template:                                              # features/time_of_day.feature:22
      """
      parsed = TimeOfDay.parse <template_time>
      tod = TimeOfDay.new(h: <h>, m: <m>, s: <s>)
      if parsed == tod
          logger.info("TimeOfDay parser is correct")
      end
      """
    When I deploy the rule                                              # features/time_of_day.feature:30
    Then It <should> log "TimeOfDay parser is correct" within 2 seconds # features/time_of_day.feature:31

    Examples: 
      | template_time | h  | m  | s  | should     |
      | '1'           | 1  | 0  | 0  | should     |
      | '02'          | 2  | 0  | 0  | should     |
      | '1pm'         | 13 | 0  | 0  | should     |
      | '12:30'       | 12 | 30 | 0  | should     |
      | '12 am'       | 0  | 0  | 0  | should     |
      | '7:00 AM'     | 7  | 0  | 0  | should     |
      | '7:00 pm'     | 19 | 0  | 0  | should     |
      | '7:30:20am'   | 7  | 30 | 20 | should     |
      | '12  am'      | 0  | 0  | 0  | should not |
      Error in script (RuntimeError)
      ./features/support/openhab.rb:36:in `check_log'
      ./features/step_definitions/openhab_rules.rb:39:in `block in deploy_rule'
      ./features/support/support.rb:5:in `block in wait_until'
      ./features/support/support.rb:4:in `times'
      ./features/support/support.rb:4:in `wait_until'
      ./features/step_definitions/openhab_rules.rb:39:in `deploy_rule'
      ./features/step_definitions/openhab_rules.rb:56:in `"I deploy the rule(:)"'
      features/time_of_day.feature:42:30:in `I deploy the rule'
      | '17:00pm'     | 17 | 0  | 0  | should not |
      Error in script (RuntimeError)
      ./features/support/openhab.rb:36:in `check_log'
      ./features/step_definitions/openhab_rules.rb:39:in `block in deploy_rule'
      ./features/support/support.rb:5:in `block in wait_until'
      ./features/support/support.rb:4:in `times'
      ./features/support/support.rb:4:in `wait_until'
      ./features/step_definitions/openhab_rules.rb:39:in `deploy_rule'
      ./features/step_definitions/openhab_rules.rb:56:in `"I deploy the rule(:)"'
      features/time_of_day.feature:43:30:in `I deploy the rule'
      | '17:00am'     | 17 | 0  | 0  | should not |
      Error in script (RuntimeError)
      ./features/support/openhab.rb:36:in `check_log'
      ./features/step_definitions/openhab_rules.rb:39:in `block in deploy_rule'
      ./features/support/support.rb:5:in `block in wait_until'
      ./features/support/support.rb:4:in `times'
      ./features/support/support.rb:4:in `wait_until'
      ./features/step_definitions/openhab_rules.rb:39:in `deploy_rule'
      ./features/step_definitions/openhab_rules.rb:56:in `"I deploy the rule(:)"'
      features/time_of_day.feature:44:30:in `I deploy the rule'

Failing Scenarios:
cucumber features/time_of_day.feature:42 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:43 # Scenario Outline: Parse strings into a TimeOfDay object
cucumber features/time_of_day.feature:44 # Scenario Outline: Parse strings into a TimeOfDay object

[boc-tothefuture]

          def self.parse(string)
            format = /(am|pm)$/i.match?(string) ? 'h[:mm[:ss]][ ]a' : 'H[:mm[:ss]]'
            puts "Time is '#{string}' Format '#{format}'"
            local_time = LocalTime.parse(string.downcase, DateTimeFormatter.ofPattern(format))
            TimeOfDay.new(h: local_time.hour, m: local_time.minute, s: local_time.second)
          rescue java.time.format.DateTimeParseException => e
            raise ArgumentError, e.message
          end

Seeing: 2021-01-12 09:05:53.247 [INFO ] [jsr223.jruby.Object ] - Processing Started for ebb9f818-26c9-40e7-8c0e-05e67b2ea64a 2021-01-12 09:05:53.250 [ERROR] [jsr223.jruby.Object ] - Error parsing time Text '1pm' could not be parsed at index 1

STDOUT: (I added a puts) Time is '1pm' Format 'h[:mm[:ss]][ ]a'

[boc-tothefuture]

The code has a downcase in it.. it seems to work if I upcase it? It looks capitalized here? https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html

Is it a locale problem in some way?

[jimtng]

The code has a downcase in it.. it seems to work if I upcase it? It looks capitalized here? https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html

Is it a locale problem in some way?

I believe you're correct.

[boc-tothefuture]

Ok to upcase or is it a locale problem? I am confused why its working on your system.

[jimtng]

I'll submit another PR

[jimtng]

Please test #64

[jimtng]

Ok to upcase or is it a locale problem?

A locale problem. Cucumber tests wouldn't have picked that up, unless we run a locale changing test. Can that be done? I'm curious what happens if you set the locale to arabic / eastern, etc.

[boc-tothefuture]

We could do it with a build matrix of some sort.

[jimtng]

Perhaps we can close this issue? If need be, open a separate issue to discuss the locale test, which I'd say is probably a low priority at this point.