MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.51k stars 1.27k forks source link

VK: Only stop Mycroft services if running in CI #2975

Closed krisgesling closed 2 years ago

krisgesling commented 3 years ago

Description

When running tests locally, a reasonable amount of time is spent starting and stopping Mycroft services. Rapidly doing this can also cause broader issues. Shutting down services at the end of a Voight Kampff test run in the CI makes sense, but it doesn't make sense for me personally when running locally.

This is the simplest implementation which provides one behaviour for CI, and the other behaviour for "not-CI". Is this sufficient or do we need to add in one or more arguments? eg:

# assume shutdown by default on all systems
mycroft-start vktest --no-stop-on-finish -t my-skill

or

# shutdown by default on CI, do not shutdown by default on non-CI but provide the option
mycroft-start vktest --stop-on-finish -t my-skill

How to test

Run some VK tests locally and see that Mycroft is still running after the tests complete. You can now run the tests repeatedly, much faster than you could previously.

Contributor license agreement signed?

devops-mycroft commented 3 years ago

Voight Kampff Integration Test Succeeded (Results)

forslund commented 3 years ago

For me this is fine.

I wonder if the start of core also should be CI only, to keep it symmetric? Though it doesn't really make much practical difference since the start will work just fine when already started.

krisgesling commented 3 years ago

I wonder if the start of core also should be CI only, to keep it symmetric? Though it doesn't really make much practical difference since the start will work just fine when already started.

Yeah I debated that in my head. My reasoning was that:

  1. you can't not start Mycroft services if you want the tests to run, and
  2. if you try start them when they're already started then the command is effectively ignored (if I'm not mistaken).
forslund commented 3 years ago

I totally buy that reasoning