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

Add 'vktest clear' command #2592

Merged
merged 2 commits into from
May 21, 2020
Merged

Add 'vktest clear' command #2592

merged 2 commits into from
May 21, 2020

Conversation

krisgesling
Copy link
Contributor

Description

Adds a new sub-command to vktest allowing developers to quickly clear any Skill Feature or Step files from the Voight Kampff directory.

To avoid duplication, all the logic is contained in bin/skill-test-runner and start-mycroft.sh calls that.

Had to do some file dancing to retain existing files. Any better suggestions welcomed...

How to test

Test a Skill then remove the files.

mycroft-start vktest -t mycroft-timer
mycroft-start vktest clear
mycroft-skill-testrunner vktest -t mycroft-weather
mycroft-skill-testrunner vktest clear

Contributor license agreement signed?

@krisgesling krisgesling requested a review from forslund May 20, 2020 13:02
@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 20, 2020
mv "$STEPS_DIR/utterance_responses.py" $TMP_DIR
rm ${STEPS_DIR}/*.py
mv ${TMP_DIR}/* $STEPS_DIR
rmdir $TMP_DIR
Copy link
Collaborator

Choose a reason for hiding this comment

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

Below is nothing that needs to happen now, just something to consider.

We will likely need to expand the defined steps and will likely add more files here and remembering to update this file list will be tricky.

Can we make a change in the test_setup to prepend a skill_ to the files it pulls in? or rely upon this being run in a git repo and do a rm $STEPS_DIR/* and then a git checkout of the folder to get back all files?

The first option will likely result in issues of double definitions of steps if there are stepfiles in there now until the files have been manually deleted. Either people will have to delete their skills manually or we can check for the old name and remove it when pulling in the stepfiles from skills.

A third option may be having this script read a "manifest.txt" file in the steps folder listing the files that should remain. since it's in the directory it's easier to remember to update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about this. At the very least I'll add a comment about what the 2 files should be in the Features dir.

should we be appending a quasi-unique identifier for the skill it came from? eg
mycroft-npr-news-skill_filename.py
Just imagining people calling their custom steps file steps.py meaning one would get overwritten.

I'm wary of baking in the expectation of it being a git repo, though it's very likely for anyone doing development.

Is there another way that test_setup could track the files it's copying in, then remove each filename from the list as they're confirmed deleted?

The manifest.txt seems like a simple middle ground.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, adding prefixes has been on my TODO for some time, collisions shouldn't occur

Copy link
Collaborator

@forslund forslund 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 and works as I'd expect. I made a note about the deleting but nothing that needs to be handled immediately.

The thing I found was that it should also remove files from the features folder ending in .config.json to handle the new custom configuration values thing.

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling requested a review from forslund May 21, 2020 10:21
@forslund
Copy link
Collaborator

Looks good. Merging.

@forslund forslund merged commit c96f546 into dev May 21, 2020
@forslund forslund deleted the feature/vk-clear branch May 29, 2020 13:52
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.

4 participants