Skip to content
This repository has been archived by the owner on Jan 27, 2020. It is now read-only.

Tests as launch actions #19

Open
hidmic opened this issue Feb 28, 2019 · 7 comments
Open

Tests as launch actions #19

hidmic opened this issue Feb 28, 2019 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@hidmic
Copy link
Contributor

hidmic commented Feb 28, 2019

Feature request

Background

Currently, tests are run on a different thread than the one the fixture's LaunchService is run. As a consequence, inspection and/or mutation of any launch object (e.g. LaunchDescriptionEntity instaces, current LaunchContext, etc.) must be guaranteed to be thread-safe. Additionally, since tests have no entity themselves in the launch fixture, some devices have to be put in place to keep the fixture alive.

Description

Run so called pre shutdown tests or a group thereof as an action within launch itself (i.e. its event loop). Tests can then interact safely with their launch fixture (e.g. actions, events, event handlers, context). Both synchronous and asynchronous tests should be supported, in case that certain condition or operation has to be waited on.

So called post shutdown tests require special treatment. Some of them may be run within a Shutdown event handler, but some may not. For the latter, either the testing framework has to take care of running them after the launch service has run or launch has to support an AfterShutdown event. In any case, this has significant implications and it is directly connected to #9.

@hidmic
Copy link
Contributor Author

hidmic commented Feb 28, 2019

I want to note that this feature request is in direct opposition to the notion of launch level tests (i.e. that run in the context of launch and interact with its context and entities) replacing/absorbing process level tests (i.e. that run in a process within the fixture and interact with it). I see several examples in this repository that exploit that idea e.g. by subscribing to a ROS topic and performing assertions on it.

I personally think they serve different purposes and should not be conflated. For instance, I'd use launch level testing to validate that a CLI is working properly and process level testing to validate that an action server is replying to requests as it should. This doesn't mean banning ROS nodes from launch level tests but to use them sparsely and only if strictly necessary.

@pbaughman I just realized this distinction might be were our differences in how we perceive and reason about launch testing may be rooted.

@pbaughman
Copy link
Contributor

@hidmic I see - yeah. The problem I set out to solve with this tool was

"We have some process level tests that are hard to implement correctly because they have no way of synchronizing with the the startup of the processes they're trying to test. What if we could also describe how the system was launched/stopped to make sure our data spoofers and driver nodes were up before we launched our process level tests"

That's a little different than the direction you're coming at this from. I'm hopeful we can make this work for both of us though!

We could shove the process level tests down into test actions that run in another process. There are some open questions about how that would look. It would also get trickier to wait on stdout, or to wait for another processes to terminate. Since some of the processes we launch (like data spoofers) don't have ROS interfaces to tell us when they're ready to go - being able to wait for stdout is a pretty important feature.

Another thing we could do is flag certain tests to run on the launch's event loop the way we do with post_shutdown_tests.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 1, 2019

That's a little different than the direction you're coming at this from. I'm hopeful we can make this work for both of us though!

It hit me yesterday. And for sure, your use case is as important as any other.

So, about synchronizing tests. One way is the one you've been pursuing i.e. push process level tests up to launch so that they can access all information. But then tests must be written in the launch system language and have to play nice with the launch system itself. Another way could be using process level tests as assertions in launch level tests, making sure that they are executed in a timely fashion e.g. wait till that process outputs some matching string, then run a test, then wait 5 seconds, then run another test. That'd remove the need for process level tests to be written in Python and to play nicely with launch. Some could even be reused as they'd probably be simpler. And as you say, we can always shove tests into actions that run on a separate process for convenience, effectively hiding the added complexity for users that do not need it.

Anyways, I'm not the voice of reason here. @wjwwood I think your opinion would be most valuable.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 11, 2019

@pbaughman are you prototyping something along (or strongly against) these lines right now? I can take a shot at it if I may.

@pbaughman
Copy link
Contributor

@hidmic Negative - have at it! I don't have many thoughts on this one, except that the IO might be a little tricky if the test results are mixed in with the other launch stuff. At some point, I wanted to change apex_launchtest_runner to be a little smarter about stdout so we didn't mix IO from launch with IO from running the tests with IO from the tests themselves.

Maybe it'll be OK, but maybe I'll have to get off my butt and do something to intercept the output from the tests that run concurrently with the launched processes so it's not all garbled together.

@pbaughman
Copy link
Contributor

@hidmic Is it going to conflict with your work if I shove the functions that bind arguments to the tests down into the test loader class?

I'm talking about taking this stuff and putting it here

I need to bind some arguments to the setUpClass, setUp, tearDown, and tearDownClass methods for the ROS examples I'm making and the apex_runner.py file is starting to be more about binding arguments than it is about running the tests

@hidmic
Copy link
Contributor Author

hidmic commented Mar 12, 2019

Not at all. Go ahead. I'm moving quite a few things around myself anyways. Thanks for the heads up!

@dejanpan dejanpan added the enhancement New feature or request label Mar 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants