Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Support tests and tasks from bundle under test #26
Conversation
bcsaller
approved these changes
Nov 30, 2016
This generally LGTM, thanks. The handling of the config file was a little confusing to me at first, but after seeing the rest take the comments or not.
| @@ -44,7 +45,10 @@ def setup(matrix, args=None): | ||
| parser.add_argument("-i", "--interval", default=5.0, type=float) | ||
| parser.add_argument("-p", "--path", default=Path.cwd() / "tests", | ||
| type=Path) | ||
| - parser.add_argument("config_file", type=open) | ||
| + parser.add_argument("-c", "--config-file", | ||
| + default=resource_filename(__name__, 'matrix.yaml')) |
bcsaller
Nov 30, 2016
Contributor
Doesn't this favor thinking about the config file as residing as part of the matrix checkout? wouldn't cwd()/tests/matrix.yaml or just ./matrix.yaml be a better choice? Also does this interfere with the #!matrix style used in some of the tests? I don't know that we need to support that mode of operations but it would make having a single exec that BT can call simple.
If this should only represent the default tests then we might go about this another way, config_fiiles with type list(path) where the default includes the resource local one?
johnsca
Nov 30, 2016
Owner
If it's not bundled as part of the Matrix, how does someone who wants to run the standard suite against their bundle under development run it?
I was also under the impression that we wanted to merge the in-bundle suite with the standard suite, rather than running it as a separate instance of Matrix nor in place of the standard suite. So, I'm not sure what the benefit of the she-bang line for matrix tests would be.
johnsca
Nov 30, 2016
Owner
This also touches on the discussion we had about whether we wanted BT to be the thing that discovers and "combines" (in some way) the in-bundle test with the standard / default case, and my understanding was that we didn't want that.
If we do, we could rip out the bundle fetch code entirely and always assume we're deploying a local bundle, with BT being responsible for fetching it and running Matrix from within the bundle directory to ensure the right paths.
| @@ -345,7 +368,7 @@ def exception_handler(self, context, loop=None, exc_ctx=None): | ||
| async def __call__(self): | ||
| btask = self.loop.create_task(self.bus.notify(False)) | ||
| - context = self.load_suite(self.config_file) | ||
| + context = await self.load_suite() |
bcsaller
Nov 30, 2016
Contributor
I appreciate the run_in_executor hurdle you jumped here (and we can keep it) but the setup code here doesn't strictly need to be async as nothing else can run when this is called.
johnsca
Nov 30, 2016
Owner
Presumably, we don't want the UI or keyboard handling to hang while it's talking to the charm store. Apparently, the CS is hitting 10s timeouts just for metadata requests occasionally, so I'm a bit wary of it being blocking.
| - deepmerge(dest[k], v) | ||
| + if isinstance(v, dict): | ||
| + deepmerge(dest.setdefault(k, {}), v) | ||
| + elif isinstance(v, list) and merge_lists: |
bcsaller
Nov 30, 2016
Contributor
Merging lists is almost never a good idea, order and uniqueness are key reasons for using lists, here we'd have to give up one or the other.
| - self.debug_view = urwid.ListBox(self.debug_walker) | ||
| - self.debug_watcher = asyncio.get_event_loop().create_task( | ||
| - self.debug_juju_log()) | ||
| + self.model_view = urwid.Terminal(['watch', '--color', '--', |
johnsca
added some commits
Nov 28, 2016
| + "a matrix.yaml file which will be included as an " | ||
| + "additional suite if present. All suites will be merged " | ||
| + "together, and suites containing tests with the same name as a " | ||
| + "test in a previous suite will override that test.", |
petevg
Nov 30, 2016
Collaborator
This gets pretty complex, so it would be nice to see some examples in this doc string.
Basically, I'm looking for a "tar xvzf" or "ps aux" -- things that a newbie can run and have some success with, even if they don't understand all the options available to them.
johnsca
added some commits
Nov 30, 2016
|
Rendered
|
|
LGTM/+1 |
johnsca commentedNov 30, 2016
Currently unable to test due to juju-solutions/python-libjuju#7 and https://bugs.launchpad.net/juju/+bug/1645917