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

Fix Voight kampff reset of patched config #2742

Merged
merged 2 commits into from
Nov 6, 2020

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Oct 28, 2020

Description

This resets a configuration patch by adding a custom cleanup function. This moves the entire config patching and resetting into the steps/configuration.py file keeping it all together in one place.

This also fixes issue with test cases following a case setting the system units would reset the config despite not needing to. This resulted in quite cluttered see here for example

How to test

Make sure VK test passes and reset message is only shown on test cases that patches the config.

Contributor license agreement signed?

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

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Oct 28, 2020
@forslund forslund added the Status: Work in progress PR being actively worked on, not yet ready for review. label Oct 28, 2020
@forslund
Copy link
Collaborator Author

As I read more of the context I see that there may be a better way of handling this. Adding the work in progress label while I research a bit more.

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling
Copy link
Contributor

Thanks for digging into this, and also for not just removing the logging line 😂

original_config will now only live in the scenario part of the context
being automatically cleared at the end of each scenario. To reset the
patch a custom cleanup is added when the patch is performed.
Since we have a reset log a matching info message for when updating the
config should be shown
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund added Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality. and removed Status: Work in progress PR being actively worked on, not yet ready for review. labels Oct 29, 2020
@forslund
Copy link
Collaborator Author

Updated the procedure to use a custom cleanup. This keeps all the config logic in a single location and fixes the redundant config patch clearing.

@forslund forslund added Type: Tests Addition or improvement of automated tests and removed Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality. labels Oct 31, 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.

context.add_cleanup() 💥

Also nice to have the config methods brought together - beautiful stuff!

@krisgesling krisgesling merged commit 8b107ce into MycroftAI:dev Nov 6, 2020
@forslund forslund deleted the bugfix/vk-reset-config-spam branch November 6, 2020 10: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) Type: Tests Addition or improvement of automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants