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

Issue-2567 - Fixing not initialized dialog_render #2685

Merged
merged 5 commits into from
Sep 15, 2020

Conversation

katridi
Copy link
Contributor

@katridi katridi commented Sep 2, 2020

Description

Fix for issue #2567

How to test

Run test/unittests/skills/test_mycroft_skill.py to ensure that test_speak_dialog_render_not_intialized won't fail

Contributor license agreement signed?

CLA [ ] (Whether you have signed a CLA - Contributor Licensing Agreement

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

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling
Copy link
Contributor

Hi Katridi, thanks for taking a look at this and submitting a fix.

I see you've added a test to test_common_query_skill.py but my understanding of the issue is that it can happen on any type of Skill. Is there a reason for it to be in the CQS tests or should this get moved to the MycroftSkill test cases?

@katridi
Copy link
Contributor Author

katridi commented Sep 3, 2020

No, you're right. I've put it in a wrong place. Thanks for pointing out!

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

krisgesling
krisgesling previously approved these changes Sep 15, 2020
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.

Thanks for switching that over. Looks good now.

Also confirmed that the test fails without the change, and passes with it applied.

@krisgesling
Copy link
Contributor

Am working on the Travis CI issue too, it has built successfully but seems to be an issue in reporting this.

@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.

I did not see that extra s before, but since we're at it, there's two tiny typo's here.

@@ -602,6 +602,13 @@ def test_translate_locations(self):
# Restore lang to en-us
s.config_core['lang'] = 'en-us'

def test_speak_dialog_render_not_intialized(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

typo on initialized

@@ -602,6 +602,13 @@ def test_translate_locations(self):
# Restore lang to en-us
s.config_core['lang'] = 'en-us'

def test_speak_dialog_render_not_intialized(self):
"""Test that non-initialzed dialog_renderer won't raise an error."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Different typo on initialized

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling merged commit a185a9a into MycroftAI:dev Sep 15, 2020
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