Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Vk error stability #2793

Merged
merged 2 commits into from
Jan 11, 2021
Merged

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Jan 6, 2021

Description

While testing #2578 we ran into a couple of issues in VK not handling some error cases in Mycroft core.

This PR ensures that

  • Missing speak meta field is handled appropriately
  • Waiting for config update times out eventually (10 seconds)

How to test

Make sure Voight Kampff tests still pass. To check the timeout comment out lines 77 and 79 of test/integrationtests/voight_kampff/features/steps/configuration.py and run the tests for the fallback-query skill.

Contributor license agreement signed?

CLA [ Yes ]

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jan 6, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

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

Looks good, two minor questions

@@ -83,8 +83,9 @@ def mycroft_responses(context):
if len(messages) > 0:
responses = 'Mycroft responded with:\n'
for m in messages:
print(m, m.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover print statement that I assume was for temporary debugging and shouldn't be logged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, will remove

expected_value (Object): The expected value indicating that the change
has been applied
"""
start_time = time.time()
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't cause a problem in this instance but I've been wondering if we should use time.monotonic() where ever possible for consistency and to avoid time sync issues?

Are there benefits of using the standard time.time()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered monotonic but thought the simplicity and readability of time.time() was better than using monotonic but I'll change it.

This can occur if a custom made speak message is injected and VK should
handle malformed speak messages.
@forslund
Copy link
Collaborator Author

Updated as requested

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling merged commit e607acd into MycroftAI:dev Jan 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants