Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix converse and snooze #110

Open
wants to merge 11 commits into
base: 20.08
Choose a base branch
from
Open

Fix converse and snooze #110

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
da6213e
fix basic converse bug and fix basic snooze functionality
ken-mycroft Mar 24, 2021
2e4cdf6
forgot vocab fix
ken-mycroft Mar 24, 2021
38abc25
hoping the absence of a Snooze.voc file does not crash non US users
ken-mycroft Mar 31, 2021
4114550
this works better but will give me grief during a PR review
ken-mycroft Apr 2, 2021
a0e1ee8
quick fix based on Gez input
ken-mycroft Apr 5, 2021
498bd86
refactor and clean up. may also require changes I made to the vktest …
ken-mycroft Apr 23, 2021
67b71d2
corrections per PR and add back in snooze tests
ken-mycroft Apr 26, 2021
5256864
fix bug where you cant exit a multi select exchange
ken-mycroft Apr 28, 2021
5994995
fix broken snooze vk tests with 10 minute period
ken-mycroft Apr 28, 2021
3fa1bde
remove file from incorrect location
ken-mycroft Apr 28, 2021
e43221b
Add #todo for consistent cancel experience
krisgesling Apr 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
270 changes: 217 additions & 53 deletions __init__.py
View file
Open in desktop

Large diffs are not rendered by default.

135 changes: 63 additions & 72 deletions test/behave/skill-alarm.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ Feature: Alarm skill functionality
| set alarm for a time |
| set alarm for 8 am |
| set an alarm for 7:30 am |
| create an alarm for 7:30 am |
| create an alarm for 7:30 pm |
| start an alarm for 6:30 am |
| let me know when it's 8:30 pm |
| wake me up at 7 tomorrow morning |

@xfail
# MS-64 https://mycroft.atlassian.net/browse/MS-64
Scenario Outline: user sets an alarm for a time
Given an english speaking user
Expand All @@ -25,8 +24,8 @@ Feature: Alarm skill functionality

Examples: user sets an alarm for a time
| set alarm for a time |
| alarm 6 pm |
| alarm for 8 tonight |
| set alarm 6:01 pm |
| alarm for 8:02 tonight |

Scenario Outline: user sets an alarm without saying a time
Given an english speaking user
Expand All @@ -39,12 +38,11 @@ Feature: Alarm skill functionality
Examples: set alarm withot saying a time
| set alarm without saying a time |
| set alarm |
| set an alarm |
| new alarm |
| create an alarm |

@xfail
# Jira MS-65 https://mycroft.atlassian.net/browse/MS-65
Scenario Outline: Failing user sets an alarm without saying a time
Scenario Outline: user sets an alarm without saying a time
Given an english speaking user
And there are no previous alarms set
When the user says "<set alarm without saying a time>"
Expand All @@ -54,10 +52,9 @@ Feature: Alarm skill functionality

Examples: set alarm withot saying a time
| set alarm without saying a time |
| set an alarm for tomorrow morning |
| set an alarm for tomorrow |
| wake me up tomorrow |
| alarm tonight |
| alarm |
| create alarm |

Scenario Outline: User sets an alarm without saying a time but then cancels
Given an english speaking user
Expand All @@ -71,7 +68,6 @@ Feature: Alarm skill functionality
| set an alarm | nevermind |
| create an alarm | cancel |

@xfail
# Jira MS-109 https://mycroft.atlassian.net/browse/MS-109
Scenario Outline: User sets an alarm without saying a time but then cancels
Given an english speaking user
Expand All @@ -94,10 +90,10 @@ Feature: Alarm skill functionality
| set a named alarm for a time |
| set an alarm named sandwich for 12 pm |
| set an alarm for 10 am for stretching |
| set an alarm for stretching 10 am |
| set an alarm for stretching 10 pm |
| set an alarm named brunch for 11 am |
| set an alarm called brunch for 11 am |
| set an alarm named workout for 11 am |
| set an alarm called brunch for 11:30 am |
| set an alarm named workout for 11 pm |

Scenario Outline: user sets an alarm without specifiying am or pm
Given an english speaking user
Expand All @@ -109,10 +105,9 @@ Feature: Alarm skill functionality
| set an alarm for a time without am or pm |
| set an alarm for 6:30 |
| set an alarm for 7 |
| wake me up at 6:30 |
| wake me up at 5:30 |
| let me know when it's 6 |

@xfail
# Jira MS-66 https://mycroft.atlassian.net/browse/MS-66
Scenario Outline: user sets an alarm without specifiying am or pm
Given an english speaking user
Expand All @@ -122,21 +117,20 @@ Feature: Alarm skill functionality

Examples: user sets an alarm without specifiying am or pm
| set an alarm for a time without am or pm |
| alarm for 12 |
| set alarm for 12 |

@xfail
# Jira MS-67 https://mycroft.atlassian.net/browse/MS-67
Scenario Outline: Failing set an alarm for a duration instead of a time
Scenario Outline: set an alarm for a duration instead of a time
Given an english speaking user
And there are no previous alarms set
When the user says "<set an alarm for a duration>"
Then "mycroft-alarm" should reply with dialog from "alarm.scheduled.for.time.dialog"

Examples:
| set an alarm for a duration |
| set an alarm for 30 minutes |
| alarm in 5 minutes |
| alarm in 5 minutes and 30 seconds |
| create an alarm for 30 minutes |
| set alarm in 5 minutes |
| new alarm in 5 minutes and 30 seconds |
Comment on lines +132 to +133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For utterances without a verb, I think we need to either:

  • create an alarm
  • catch the utterance and disambiguate

Currently the Alarm Skill won't trigger from this utterance and so you would either get the Date Time Skill or a fallback even though we have the keyword alarm and a time / duration.

My assumption is that this should be a Padatious intent so it only matches specific short utterances and we don't have any utterance with the word "alarm" triggering this Skill.
eg

alarm
alarm in {duration}
alarm at {time}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, where to start. First, these tests all pass so the behavior Derick requested was achieved. The utterance 'alarm 6' will now create an alarm at 6. Second, indeed, this triggers for nearly anything which contains the word 'alarm' in it. I don't know if that's a good or bad thing as we are reaching the point of 'the little boy and the dyke' mode where you catch something new here and lose something old there. This will get worse before it gets better with our current design which is why I push for new requirements to be specified using VK tests. This will be the only hope we have of catching intent conundrums as we move forward. How that squares with reducing test coverage by removing tests to decrease test execution times is left as an exercise for the reader.

| set an alarm 8 hours from now |
| set an alarm 8 hours and 30 minutes from now |

Expand All @@ -163,24 +157,24 @@ Feature: Alarm skill functionality
Examples: set a recurring alarm
| set a recurring alarm for a time |
| set alarm every weekday at 7:30 am |
| wake me up every weekday at 7:30 am |
| wake me up every weekday at 4:30 am |
| set an alarm every wednesday at 11 am |
| set an alarm for weekends at 3 pm |
| set an alarm for 3 pm every weekend |
| set an alarm for 1 pm every weekend |

@xfail
# Jira 68 https://mycroft.atlassian.net/browse/MS-68
Scenario Outline: Failing user sets a recurring alarm
Scenario Outline: user sets a recurring alarm
Given an english speaking user
And there are no previous alarms set
When the user says "<set a recurring alarm for a time>"
Then "mycroft-alarm" should reply with dialog from "recurring.alarm.scheduled.for.time.dialog"

# removed because not currently supported
# | wake me up every day at 5:30 am except on the weekends |
Examples: set a recurring alarm
| set a recurring alarm for a time |
| set alarm every weekday at 7:30 am |
| alarm every weekday at 7:30 am |
| wake me up every day at 7:30 am except on the weekends |
| alarm every weekday at 6:30 am |

Scenario Outline: user sets a recurring alarm without saying a time
Given an english speaking user
Expand Down Expand Up @@ -251,9 +245,8 @@ Feature: Alarm skill functionality
| when will my alarm go off |
| when's my alarm |

@xfail
# Jira 69 https://mycroft.atlassian.net/browse/MS-69
Scenario Outline: Failing user asks for alarm status of a single alarm
Scenario Outline: user asks for alarm status of a single alarm
Given an english speaking user
And there are no previous alarms set
And an alarm is set for 9:00 am on weekdays
Expand Down Expand Up @@ -285,9 +278,8 @@ Feature: Alarm skill functionality
| when will my alarm go off |
| when's my alarm |

@xfail
# Jira 70 https://mycroft.atlassian.net/browse/MS-70
Scenario Outline: Failing user asks for alarm status of multiple alarms
Scenario Outline: user asks for alarm status of multiple alarms
Given an english speaking user
And there are no previous alarms set
And an alarm is set for 9:00 am on weekdays
Expand Down Expand Up @@ -318,9 +310,8 @@ Feature: Alarm skill functionality
| when will my alarm go off |
| when's my alarm |

@xfail
# Jira MS-71 https://mycroft.atlassian.net/browse/MS-71
Scenario Outline: Failing user asks for alarm status when no alarms are sets
Scenario Outline: user asks for alarm status when no alarms are sets
Given an english speaking user
And there are no previous alarms set
When the user says "<alarm status>"
Expand All @@ -344,6 +335,7 @@ Feature: Alarm skill functionality
Examples: stop beeping
| stop |
| stop |
| kill |
| stop alarm |
| disable alarm |
| cancel |
Expand All @@ -355,42 +347,6 @@ Feature: Alarm skill functionality
| abort |
| kill alarm |

@xfail
# Jira MS-72 https://mycroft.atlassian.net/browse/MS-72
Scenario Outline: user snoozes a beeping alarm
Given an english speaking user
And there are no previous alarms set
And an alarm is expired and beeping
When the user says "<snooze>"
Then "mycroft-alarm" should stop beeping and start beeping again 10 minutes

Examples: snooze a beeping alarm
| snooze |
| snooze |
| snooze alarm |
| not yet |
| 10 more minutes |
| 10 minutes |
| snooze for 10 minutes |
| give me 10 minutes |
| wake me up in 10 minutes |
| remind me in 10 minutes |
| let me sleep |

@xfail
# Jira MS-73 https://mycroft.atlassian.net/browse/MS-73
Scenario Outline: user snoozes an beeping alarm for a specific time
Given an english speaking user
And there are no previous alarms set
And an alarm is expired and beeping
When the user says "<snooze for a time>"
Then "mycroft-alarm" should stop beeping and start beeping again 5 minutes

Examples: snooze a beeping alarm for a specific time
| snooze for a time |
| snooze for 5 minutes |
| give me 10 minutes |

Scenario Outline: user deletes an alarm when a single alarm is active
Given an english speaking user
And there are no previous alarms set
Expand Down Expand Up @@ -427,7 +383,6 @@ Feature: Alarm skill functionality
| cancel |


@xfail
# Jira MS-74 https://mycroft.atlassian.net/browse/MS-74
Scenario Outline: user deletes an alarm when multiple alarms are active
Given an english speaking user
Expand All @@ -451,7 +406,6 @@ Feature: Alarm skill functionality
| abort alarm |
| remove alarm |

@xfail
# Jira MS-75 https://mycroft.atlassian.net/browse/MS-75
Scenario Outline: user deletes a specific alarm
Given an english speaking user
Expand Down Expand Up @@ -494,3 +448,40 @@ Feature: Alarm skill functionality
| remove all alarms |
| remove every alarm |
| delete every alarm |

# Jira MS-72 https://mycroft.atlassian.net/browse/MS-72
Scenario Outline: user snoozes a beeping alarm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These significantly increase the time it takes for these tests to run. So before this gets merged into the marketplace we really need to think through which tests are going to run when, and how we define that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I considered adding configurable ability for snooze so we could drop it to like 1 minute during testing but there are only so many hours in a day. I am not adverse to removing the snooze tests or any other tests causing excessive execution time but losing tests does not feel right.

Given an english speaking user
And there are no previous alarms set
And an alarm is expired and beeping
When the user says "<snooze>"
Then "mycroft-alarm" should stop beeping and start beeping again in 10 minutes

Examples: snooze a beeping alarm
| snooze |
| snooze |
| snooze alarm |
| not yet |
| 10 more minutes |
| 10 minutes |
| snooze for 10 minutes |
| give me 10 minutes |
| wake me up in 10 minutes |
| remind me in 10 minutes |
| let me sleep |

# Jira MS-73 https://mycroft.atlassian.net/browse/MS-73
Scenario Outline: user snoozes an beeping alarm for a specific time
Given an english speaking user
And there are no previous alarms set
And an alarm is expired and beeping
When the user says "<snooze for a time>"
Then "mycroft-alarm" should stop beeping and start beeping again in 5 minutes

Examples: snooze a beeping alarm for a specific time
| snooze for a time |
| snooze for 5 minutes |
| give me 5 minutes |
| snooze 5 |
| snooze for 5 |

Loading