Make consistence over all steps

[damorosodaragona]

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

[devs-mycroft]

Hello, @damorosodaragona, thank you for helping with the Mycroft project! We welcome everyone into the community and greatly appreciate your help as we work to build an AI for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require a Contributor Licensing Agreement (CLA) before accepting any code contribution. This agreement makes it crystal clear that along with your code you are offering a license to use it within the confines of this project. You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank you!

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[forslund]

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.

[devops-mycroft]

Voight Kampff Integration Test Failed (Results)

[pep8speaks]

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! :beers:

Comment last updated at 2020-10-01 20:47:07 UTC

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[devops-mycroft]

Voight Kampff Integration Test Succeeded (Results)

[krisgesling]

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:

Then "mycroft-timer" reply should contain "10 minutes"

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.

[damorosodaragona]

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

@then('"{skill}" should reply with exactly "{text}"')
def then_exactly(context, skill, text):
    def check_exact_match(message):
        utt = message.data['utterance'].lower()
        debug = 'Comparing {} with expected {}\n'.format(utt, text)
        result = utt == text.lower()
        return (result, debug)

    passed, debug = then_wait('speak', check_exact_match, context)
    if not passed:
        assert_msg = debug
        assert_msg += mycroft_responses(context)
    assert passed, assert_msg

Should pass if any skill answer with {text}. So given your example if i say:

Then "mycroft-timer" should reply with exactly "10 minutes"

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

[forslund]

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

[damorosodaragona]

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 Then {skill} reply should contain.

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.

[krisgesling]

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 {skill name} will have then this is highly likely to break once the functionality is actually added.

I'm open to other opinions on this though.

[krisgesling]

Seems to be blocked until we have an agreed upon skill_name as per issue #2891