MycroftAI / mycroft-timer

Mycroft AI official Timer Skill - set multiple named timers
https://mycroft.ai/skills
Apache License 2.0
7 stars 30 forks source link

Test: Make check for timer not stopping event driven #101

Closed forslund closed 3 years ago

forslund commented 3 years ago

Description

This is an option to make the "still beeping" check suggested by @chrisveilleux event-driven.

Pros: Kind of simple

Cons: Looks at an intermediary bus message instead of checking the actual playback.

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR. Either delete those that do not apply, or add an x between the square brackets like so: - [x]

Testing

Run the VK tests for the skill make sure that some of the cancel expired timer tests works. There are some of those tests that uses unrecognized vocabulary (at least on my machine) so all till not pass. But make sure if the vocab fails the test should also fail.

krisgesling commented 3 years ago

Tests seem to be passing with the process.wait :tada:

I still think something like this would be beneficial but if we're going to implement something around sound effects it may be worth holding off?

forslund commented 3 years ago

I'm not sure how important it is. Current solution with the changes made to the skill may give false positives (passing tests where they should actually be failing). The liklyhood of false positive should be something like 1 - (length of audio / length of delay beween start of play), but again it's a false positive so not the end of the world. Especially if the sound effect service is something that's in the near future

forslund commented 3 years ago

Closing this, assuming we'll hold off until a sfx-service is added.