Allow parametrization to test descriptions #39

pbaughman · 2019-04-04T22:14:24Z

Allow the launch to be parametrized, so that one call to apex_launchtest can cause several launches to happen.

Architecture

When we load the module, resolve any parametrization of the generate_test_description function. We can load the tests at this point too, instead of waiting until the run starts. The ApexRunner class is now responsible for looping over all of the test runs and combining the results. Running the launch and running the tests is now delegated to a private class called the RunnerWorker.

Side Note: Once this is done, it will make Issue 9 a lot easier to complete. We'll just need the loader to produce multiple test runs for the tests that want to run in isolation, creating a new LaunchDescription every time.

Todo

Run multiple launches when parametrized
Make sure tests names are updated so we can tell which tests ran with which parameters
Combine results from multiple test runs
Add examples for parametrized launches
Signed-off-by: Pete Baughman pete.baughman@apex.ai

pbaughman · 2019-04-05T01:05:49Z

@hidmic For a black-box view of how this is going to work, take a look at https://github.com/ApexAI/apex_rostest/blob/399128c6a4cdb2a4c9f07430c0c946ac7025e558/apex_launchtest/examples/parameters.test.py

This example runs, but it exits after the 1st parameter because i have no code to combine the results from multiple runs yet. The early-out is here: https://github.com/ApexAI/apex_rostest/pull/39/files#diff-aaece43019a305b833d34efac4246041R220

pbaughman · 2019-04-08T20:53:29Z

@hidmic I got the parametrized example running today, but I'm seeing some exceptions coming out of launch. I will dig in but thought maybe you might recognize the stack-trace and instantly know what's going on. It doesn't happen every time:

> apex_launchtest apex_launchtest/examples/parameters.test.py
. . .
ERROR:asyncio:Task exception was never retrieved
future: <Task finished coro=<LaunchService._process_one_event() done, defined at /opt/ApexOS/lib/python3.5/site-packages/launch/launch_service.p
y:177> exception=ProcessLookupError(3, 'No such process')>
Traceback (most recent call last):
  File "/usr/lib/python3.5/asyncio/tasks.py", line 239, in _step
    result = coro.send(None)
  File "path/lib/python3.5/site-packages/launch/launch_service.py", line 179, in _process_one_event
    await self.__process_event(next_event)
  File "path/lib/python3.5/site-packages/launch/launch_service.py", line 199, in __process_event
    visit_all_entities_and_collect_futures(entity, self.__context))
  File "path/lib/python3.5/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 38, in visit_all_entities
_and_collect_futures
    sub_entities = entity.visit(context)
  File "path/lib/python3.5/site-packages/launch/action.py", line 59, in visit
    return cast(Optional[List[LaunchDescriptionEntity]], self.execute(context))
  File "path/lib/python3.5/site-packages/launch/actions/opaque_function.py", line 74, in execute
    return self.__function(context, *self.__args, **self.__kwargs)
  File "path/lib/python3.5/site-packages/launch/actions/execute_process.py", line 269, in __on_signal_process_event
    self._subprocess_transport.send_signal(typed_event.signal)
  File "/usr/lib/python3.5/asyncio/base_subprocess.py", line 146, in send_signal
    self._proc.send_signal(signal)
  File "/usr/lib/python3.5/subprocess.py", line 1777, in send_signal
    os.kill(self.pid, sig)
ProcessLookupError: [Errno 3] No such process

I use a fresh launch service for each run, but I know there's some global state in there - maybe something is preventing us from calling launch.run successfully multiple times per process?

One other wrinkle - I think our version of 'launch' is from ROS crystal

hidmic · 2019-04-08T21:32:17Z

@pbaughman that used to happen when a signal was sent after the process had died, due to a race condition in the ExecuteProcess action. We fixed that one upstream.

- Add the parametrization infrastrcture - Refactor the apex runner into an ApexRunner and a RunnerWorker - This veresion should still run existing tests. This is mostly a refactor commit Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

- Combine pre and post results into one set of results, add class names so the tests are distinguishable - Update tests to match new 'run' API that returns a dictionary of {test_run: result} instead of a tuple of (pre_shutdown, post_shutdown) results

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

hidmic · 2019-04-10T20:48:02Z

@pbaughman I've been pulling this branch, as well as other recent changes in this repository, back into core launch repositories to try this out. It looks like argument and attribute binding for post shutdown tests parameterized on the launch description is broken though. Can you confirm that's the case? Either that or I just messed up with all the cherry-picking and squashing I have been doing. :)

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-04-10T22:56:52Z

@hidmic I think it works - I just pushed a commit that adds a post_shutdown_test to examples/parameters.test.py and it seems to run OK.

The magic happens here which I'm sure was a conflict-heavy file, unfortunately. The code to bind to the post shutdown tests should be almost identical to the pre-shutdown tests right above.

The main difference between the binding above and the binding below is we give proc_info and proc_output objects that don't have the 'assertWaitFor' methods, because it doesn't make sense to wait for IO or process shutdown after all of the processes have shut down. I wanted to give a good error message if someone waited for IO from a post-shutdown test (but I don't think I ever implemented the good error message)

- Print an extra line that syas "Starting test run . . . " to show which args we're running with - Update the test names the same way pytest does - by appending '[value_of_arg, value_of_arg2, . . ]' - This information makes it into the junit XML too Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

hidmic · 2019-04-11T21:18:11Z

Alright, nevermind, found the problem. Parameterization support works just fine.

pbaughman · 2019-04-16T21:18:37Z

@hidmic Ok, I think I've added everything I want to add to this. If you want to add feedback, this isn't going to change out from under you anymore.

I'll also grab someone from Apex to look at it too

pbaughman · 2019-04-16T21:21:30Z

apex_launchtest/apex_launchtest/apex_launchtest_main.py

@@ -29,7 +30,10 @@
 def _load_python_file_as_module(python_file_path):
    """Load a given Python launch file (by path) as a Python module."""
    # Taken from apex_core to not introduce a weird dependency thing
-    loader = SourceFileLoader('python_launch_file', python_file_path)
+    loader = SourceFileLoader(
+        os.path.basename(python_file_path),


This will give the tests a better name in the resulting XML. The TestRun class in loader.py uses the module name to construct the name of the test run. This information eventually makes it into the junit XML file and makes it a little easier to see where tests came from, and what parameters they ran with.

pbaughman · 2019-04-16T21:23:23Z

apex_launchtest/apex_launchtest/apex_runner.py

-            return result
-        else:
-            return result, {}
+class _LaunchDiedException(Exception):


The launch can die out from under you because of an error in the launch description, because all of the process accidentally died, or because the user hit ctrl+c. With the ApexRunner split into _RunnerWorker and ApexRunner I needed a way to communicate up to the ApexRunner that this happened.

pbaughman · 2019-04-16T21:26:53Z

apex_launchtest/apex_launchtest/apex_runner.py

-        )(lambda: self._processes_launched.set())
-
-        # Data to squirrel away for post-shutdown tests
-        self.proc_info = ActiveProcInfoHandler()


Since we're doing 100% of the loading and binding before any tests run now, we can reduce the scope of the proc_into and proc_output helper objects.

Previously, we delayed loading the tests until we were ready to run them, but the new TestRun object over in loader.py makes it easier to do the loading and binding and parametrization up front

pbaughman · 2019-04-16T21:28:27Z

apex_launchtest/apex_launchtest/loader.py

@@ -34,19 +137,6 @@ def loadTestsFromTestCase(self, testCaseClass):

            if getattr(testCaseClass, '__post_shutdown_test__', False) == load_post_shutdown:
                cases = super(_loader, self).loadTestsFromTestCase(testCaseClass)
-


This is all part of 'bind' on the TestRun object now - up above about 30 lines

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman · 2019-04-16T22:13:36Z

@jpsamper2009 or @left4taco Can one of you review this?

t0ny-peng · 2019-04-16T22:32:20Z

@jpsamper2009 or @left4taco Can one of you review this?

Is this PR still WIP? If not, shall we remove the WIP in the title?

pbaughman · 2019-04-16T23:49:37Z

@left4taco Whoops - fixed sorry

t0ny-peng

Good design. I left some of my questions. But those are quite trivial.
Thanks.

t0ny-peng · 2019-04-18T23:20:48Z

apex_launchtest/apex_launchtest/loader.py

@@ -34,19 +137,6 @@ def loadTestsFromTestCase(self, testCaseClass):

            if getattr(testCaseClass, '__post_shutdown_test__', False) == load_post_shutdown:


If a test case class has attribute __post_shutdown_test__, I assume it could only be True, right? For non-post_shut_down_test, they don't have this attribute at all.
Can we use has_attr here?

t0ny-peng · 2019-04-18T23:43:33Z

apex_launchtest/apex_launchtest/apex_runner.py

-            self.test_args[k] = v
+            test_args[k] = v
+
+        self._test_run.bind(


Is it possible to change _test_run to another name? There's already a _run_test_ function below. Might cause confusion.

Also the plural _test_runs

t0ny-peng · 2019-04-18T23:52:52Z

apex_launchtest/apex_launchtest/test_result.py

@@ -64,3 +64,13 @@ def startTest(self, test):
    def stopTest(self, test):
        self.__test_cases[test]['end'] = time.time()
        super().stopTest(test)
+
+    def append(self, results):


Might not be proper to use TestResult as the class name as it's the same with unittest.TestResult

apex_launchtest/apex_launchtest/test_result.py

pbaughman self-assigned this Apr 4, 2019

pbaughman mentioned this pull request Apr 4, 2019

Ability to run tests with a freshly launched system #9

Open

pbaughman force-pushed the launch_description_params branch from 83d860b to 399128c Compare April 5, 2019 00:57

pbaughman force-pushed the launch_description_params branch from 399128c to 83444bc Compare April 8, 2019 20:48

pbaughman force-pushed the launch_description_params branch from 83444bc to 7e2da27 Compare April 8, 2019 21:01

pbaughman force-pushed the launch_description_params branch 3 times, most recently from 78171d6 to 71bb59d Compare April 8, 2019 22:59

Pete Baughman added 2 commits April 8, 2019 16:08

Beef up test coverage for new features

d028b62

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman force-pushed the launch_description_params branch from 71bb59d to d028b62 Compare April 8, 2019 23:08

Improve test to show post_shutdown_test parametrization works too

5a8dee2

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman force-pushed the launch_description_params branch 2 times, most recently from 09baebc to 35d0dc3 Compare April 16, 2019 21:17

pbaughman commented Apr 16, 2019

View reviewed changes

Better error messages for bad parametrization

f5a9c2f

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

pbaughman force-pushed the launch_description_params branch from 35d0dc3 to f5a9c2f Compare April 16, 2019 21:35

pbaughman changed the title ~~WIP Allow parametrization to test descriptions~~ Allow parametrization to test descriptions Apr 16, 2019

pbaughman added enhancement New feature or request Needs Upstream Merge For PRs merged after apex_rostest was copied to ros2/launch labels Apr 17, 2019

pbaughman mentioned this pull request Apr 17, 2019

Improve handling of skipped tests #41

Merged

t0ny-peng approved these changes Apr 19, 2019

View reviewed changes

pbaughman merged commit 5afad31 into master Apr 19, 2019

pbaughman deleted the launch_description_params branch April 19, 2019 00:19

pbaughman mentioned this pull request Apr 19, 2019

Parametrize the test description #38

Closed

5 tasks

pbaughman removed the Needs Upstream Merge For PRs merged after apex_rostest was copied to ros2/launch label May 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow parametrization to test descriptions #39

Allow parametrization to test descriptions #39

pbaughman commented Apr 4, 2019 •

edited

Loading

pbaughman commented Apr 5, 2019 •

edited

Loading

pbaughman commented Apr 8, 2019 •

edited

Loading

hidmic commented Apr 8, 2019

hidmic commented Apr 10, 2019 •

edited

Loading

pbaughman commented Apr 10, 2019

hidmic commented Apr 11, 2019

pbaughman commented Apr 16, 2019

pbaughman Apr 16, 2019

pbaughman Apr 16, 2019

pbaughman Apr 16, 2019

pbaughman Apr 16, 2019

pbaughman commented Apr 16, 2019

t0ny-peng commented Apr 16, 2019

pbaughman commented Apr 16, 2019

t0ny-peng left a comment

t0ny-peng Apr 18, 2019

t0ny-peng Apr 18, 2019

t0ny-peng Apr 18, 2019

		@@ -34,19 +137,6 @@ def loadTestsFromTestCase(self, testCaseClass):

		if getattr(testCaseClass, '__post_shutdown_test__', False) == load_post_shutdown:
		cases = super(_loader, self).loadTestsFromTestCase(testCaseClass)

Allow parametrization to test descriptions #39

Allow parametrization to test descriptions #39

Conversation

pbaughman commented Apr 4, 2019 • edited Loading

Architecture

Todo

pbaughman commented Apr 5, 2019 • edited Loading

pbaughman commented Apr 8, 2019 • edited Loading

hidmic commented Apr 8, 2019

hidmic commented Apr 10, 2019 • edited Loading

pbaughman commented Apr 10, 2019

hidmic commented Apr 11, 2019

pbaughman commented Apr 16, 2019

pbaughman Apr 16, 2019

Choose a reason for hiding this comment

pbaughman Apr 16, 2019

Choose a reason for hiding this comment

pbaughman Apr 16, 2019

Choose a reason for hiding this comment

pbaughman Apr 16, 2019

Choose a reason for hiding this comment

pbaughman commented Apr 16, 2019

t0ny-peng commented Apr 16, 2019

pbaughman commented Apr 16, 2019

t0ny-peng left a comment

Choose a reason for hiding this comment

t0ny-peng Apr 18, 2019

Choose a reason for hiding this comment

t0ny-peng Apr 18, 2019

Choose a reason for hiding this comment

t0ny-peng Apr 18, 2019

Choose a reason for hiding this comment

pbaughman commented Apr 4, 2019 •

edited

Loading

pbaughman commented Apr 5, 2019 •

edited

Loading

pbaughman commented Apr 8, 2019 •

edited

Loading

hidmic commented Apr 10, 2019 •

edited

Loading