-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Make consistence over all steps #2711
base: dev
Are you sure you want to change the base?
Conversation
The step contains is the only step where is necessary write "mycroft" and not "{skill"}. So for all the step is necessary write something like 'Then "myskill" should reply with...' except for the should contains step were is necessary write: 'Then mycroft reply should contains... " and not 'Then" myskill" reply should contains... ".
Hello, @damorosodaragona, thank you for helping with the Mycroft project! We welcome everyone To protect yourself, the project, and users of Mycroft technologies we require Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank |
Voight Kampff Integration Test Succeeded (Results) |
Since this is used currently by existing skills the old version needs to remain in addition to this new one. I think it should be as easy as having both the old and the new decorator on the function. |
Voight Kampff Integration Test Failed (Results) |
Backward compatibility changing then_contains step.
Hello @damorosodaragona! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-10-01 20:47:07 UTC |
Voight Kampff Integration Test Succeeded (Results) |
1 similar comment
Voight Kampff Integration Test Succeeded (Results) |
Hey @damorosodaragona thanks for looking into this further. Please correct me if I'm wrong, but it doesn't seem like this will check that the specific Skill defined in the test generated the dialog. So if I say:
the test would pass if any Skill replied with an utterance containing "10 minutes". Personally I think the expectation would be that it would test both the Skill and the utterance. |
@krisgesling yes, you're right. But looking in the code it seems that not all the steps that required the skill name use it. Coreect me if i'm wrong: this step does not use skill name:
Should pass if any skill answer with {text}.
The test could pass if any skill reply with "10 minutes" . This is an example but in my impression also others steps that use skill name work like this, so without using skill name |
You are correct, The actual checking of skill source isn't implemented basically due to an on-going discussion (at least it was back when these were implemented), determining the format of the skill name. skill-id is what is reported in the speak meta data but it isn't quite friendly for writing the behave files. So some sort of translation is needed skill-id -> pretty name. I'll add an issue reflecting this |
So i think that we can standardize also the use of then_contain (and maybe also then_message type) adding, as in this pull request, the possibility to write When the {skill} check will be implemented, will be necessary only split the two steps, one allowing the use of {skill} to say that the {skill} reply has to containt a specific word and onother one step allowing use "mycroft" to say that is not important which skill reply but the reply should contain the specific word. But now, since there is not a very different behaviour using or not skill name on a step, i think we can standardize the steps, so as to no getting in confusion. |
I agree that we should standardize however I'd rather not add to breaking changes down the track if we can avoid it. Even if it's very clear for Skill authors that the intention behind the Step is that in the future it will check which Skill generated the dialog, if we aren't sure about the format that the I'm open to other opinions on this though. |
Seems to be blocked until we have an agreed upon |
The step contains is the only step where is necessary write "mycroft" and not "{skill"}.
So for all the step is necessary write something like
'Then "myskill" should reply with...'
except for the should contains step were is necessary write: 'Then mycroft reply should contains... " and not
'Then" myskill" reply should contains...' .
Description
(Description of what the PR does, such as fixes # {issue number})
How to test
(Description of how to validate or test this PR)
Contributor license agreement signed?
CLA [ ] (Whether you have signed a CLA - Contributor Licensing Agreement