First prototype of native pytest plugin for launch based tests #528

ivanpauno · 2021-09-07T19:09:28Z

Some early discussion can be found here: https://gist.github.com/ivanpauno/761a7cbca83335ea081512f3494304ba.

I'm planning to add support for shutdown tests.
There are also a lot of other details that can be discussed.

hidmic

First pass, great stuff!

launch_testing/launch_testing/pytest/plugin.py

hidmic · 2021-09-07T20:00:31Z

launch_testing/launch_testing/pytest/fixture.py

+                ):
+                    mod_locals[name] = getter(scope)
+            else:
+                mod_locals[name] = getter(scope)


@ivanpauno I think I understand the override-able fixture magic, but doesn't this imply fixtures of different scopes cannot coexist in the same source file? I mean, you could have 3 tests using a module scope fixture and 3 tests using a function scope fixture and there wouldn't be any overlap, would it?

If you have one launch_test fixture with class scope and one with function scope, the launch_service function will be class scoped.

This is required because of the following issue:

@pytest.fixture() def foo(): ... @pytest.fixture(scope='module') def bar(foo): ...

In that case you get a "scope mismatch error", because you cannot access a fixture with a lower scope from another fixture.

The fact that the launch_service/event_loop is automatically redefined with a larger scope only means that it might be shared by both launch_test fixtures, which is not necessarly a problem.
We could actually create one launch_service per launch_test fixture, and don't use a fixture for that at all.
For the event_loop though, you can only have one active event loop in a thread, so it would be an issue anyway.

Maybe in the future pytest solves this issue, see pytest-dev/pytest#6101.

In that case you get a "scope mismatch error", because you cannot access a fixture with a lower scope from another fixture.

Aye, the launch_service fixture scope has to be at least as large as the largest scope of all launch fixtures found (BTW isn't that example reversed?).

The fact that the launch_service/event_loop is automatically redefined with a larger scope only means that it might be shared by both launch_test fixtures, which is not necessarly a problem.

I see two problems. Unrelated larger scope fixtures creeping into tests with smaller scope fixtures, and a large scope launch fixture being shutdown (which IIRC is a terminal state) by a smaller scope fixture upon finalization. The first one is not really a problem if we enforce a 1-to-1 relationship between tests and launch fixtures. The second one is.

We could actually create one launch_service per launch_test fixture, and don't use a fixture for that at all.

Having separate instances is probably easiest, yeah. Alternatively, we can make LaunchService shutdowns not a terminal state (i.e. allow re-running after a shutdown).

(BTW isn't that example reversed?).

lol yes, fixed.

and a large scope launch fixture being shutdown (which IIRC is a terminal state) by a smaller scope fixture upon finalization

Yes, I think we can detect that case and warn the user though.

wjwwood

I left a few comments, but honestly the pytest stuff is a bit over my head (opaque without some special knowledge it seems).

launch/launch/launch_service.py

launch_testing/launch_testing/pytest/plugin.py

launch_testing/test/launch_testing/new_hooks/test_function_scope.py

hidmic · 2021-09-07T21:39:04Z

On a related note, would it make sense to roll out a separate launch_pytest package? Imports would be shorter and it's easier to draw a line between this new thing and launch_testing. WDYT @ivanpauno @wjwwood @jacobperron ?

ivanpauno · 2021-09-08T13:49:13Z

On a related note, would it make sense to roll out a separate launch_pytest package? Imports would be shorter and it's easier to draw a line between this new thing and launch_testing. WDYT @ivanpauno @wjwwood @jacobperron ?

The imports wouldn't really be much shorter.
Having a separate package sounds fine to me, I actually started that way and then integrated the changes in launch_testing.

There're a few things that I can reuse from the current launch_testing though, like the ReadyToTest action.
I'm not sure if we can also take advantage of some of the existent tools to e.g. launch a new process, capture process stdout, etc.

hidmic · 2021-09-08T15:37:07Z

I'm not sure if we can also take advantage of some of the existent tools to e.g. launch a new process, capture process stdout, etc.

Maybe, to some extent. We can probably do better but it's not a bad start.

launch_testing/launch_testing/pytest/plugin.py

ivanpauno · 2021-09-09T19:07:52Z

@hidmic could you take another look?

I have added support for shutdown tests, including generator/asyncgenerators based ones.
The code needs more polishing, more tests, and maybe we should handle launch_service higyene in a better way; but I don't think we need many more features (except for improving tools to expect for process output/launch process/etc, but that's kind of unrelated).

launch_testing/launch_testing/pytest/plugin.py

hidmic

Alright, I left some comments, questions, suggestions, and ideas. This is complex code, sorry if I mixed things up 😅

launch_testing/launch_testing/pytest/plugin.py

hidmic · 2021-09-09T22:13:00Z

launch_testing/launch_testing/pytest/plugin.py

+                args[name] = value
+        if inspect.iscoroutinefunction(func):
+            pyfuncitem.obj = wrap_coroutine(func, args, event_loop, before_test)
+        elif inspect.isgeneratorfunction(func):


@ivanpauno meta: does it make sense to have pure post-shutdown tests that are sync or async generators?

Not really, I think the shutdown flag is being ignored in that case, though maybe better we should show an error.
I'm also having problems to support pure post-shutdown tests, so maybe I'll delete that option altogether.

See bf8c790

launch_testing/launch_testing/pytest/plugin.py

clalancette · 2021-09-14T20:01:26Z

On a related note, would it make sense to roll out a separate launch_pytest package? Imports would be shorter and it's easier to draw a line between this new thing and launch_testing. WDYT @ivanpauno @wjwwood @jacobperron ?

The imports wouldn't really be much shorter.
Having a separate package sounds fine to me, I actually started that way and then integrated the changes in launch_testing.

On the one hand, replacing launch_testing completely with the new pytest-based thing is where we eventually want to be. On the other hand, just replacing all of the guts of launch_testing is probably going to break all downstream packages that have managed to use launch_testing successfully.

Because of that, my inclination is to make a new package, and once that is in have import launch_testing raise a DeprecationWarning in Humble. Then for I-Turtle we can remove the old launch_testing. What do you think about that plan?

On a different meta-note, I'd also like to see some examples on how to use this new functionality in downstream packages. For instance, how would I change https://github.com/ros-teleop/teleop_tools/blob/foxy-devel/joy_teleop/test/test_action_fibonacci.py to use this new package? Maybe those examples should be in here, or maybe they should be over in https://github.com/ros2/examples, or maybe they should be in the documentation.

ros-discourse · 2021-09-20T20:53:03Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1

wjwwood · 2021-09-24T17:45:03Z

On the one hand, replacing launch_testing completely with the new pytest-based thing is where we eventually want to be. On the other hand, just replacing all of the guts of launch_testing is probably going to break all downstream packages that have managed to use launch_testing successfully.

I'm fine with this approach, to be honest. It also opens us up to being able to do launch_next_new_cool_testing_thing down the line when that comes around.

hidmic

Second (or third?) pass!

launch_testing/launch_testing/pytest/fixture.py

hidmic · 2021-10-04T21:04:21Z

launch_testing/launch_testing/pytest/plugin.py

+    raise RuntimeError(
+        'launch_pytest fixtures must return a LaunchDescription '
+        'containing only one ReadyToTest action'
+    )


@ivanpauno meta: just like launch_testing, this cannot really detect all ReadyToTest instances (thinking of includes). We need to document the fact that we'll simply pick up the first one found we can statically find. In which case, I'm slightly inclined to make this a warning. Thoughts?

A warn would be fine as well.

launch_testing/launch_testing/pytest/plugin.py

launch_testing/test/launch_testing/new_hooks/test_function_scope.py

hidmic · 2021-10-04T21:39:16Z

launch_testing/test/launch_testing/new_hooks/test_function_scope.py

+    return launch.LaunchDescription([
+        launch_testing.util.KeepAliveProc(),
+        launch_testing.actions.ReadyToTest(),
+    ])


@ivanpauno It'd be nice to test a launch_description that yields (though I don't see why it wouldn't work)

launch_testing/launch_testing/util/__init__.py

launch_testing/launch_testing/pytest/plugin.py

ivanpauno · 2021-10-14T20:38:57Z

FYI, force-pushed to rebase with master

ivanpauno · 2021-10-19T21:28:32Z

The reason why the Rpr checker fails is interesting.

It seems that we're using the pip installed pytest in the buildfarm, due to pytest-rerunfailures being installed from pip (no debian package available in Focal).
pytest-asyncio from python3-pytest-asyncio (via rosdep), and the version available in ubuntu debians is not compatible with the pip installed pytest.

I'm not using much of pytest-asyncio, only a fixture that I can duplicate in our plugin, but the issue is a bit unfortunate.

ivanpauno · 2021-10-19T21:29:28Z

Note to self: Make sure our plugin works with both the pip and debian package version of pytest.

ivanpauno · 2021-10-19T21:44:02Z

I'm not using much of pytest-asyncio, only a fixture that I can duplicate in our plugin, but the issue is a bit unfortunate.

Issue fixed in 8514c4e

hidmic

This is amazing @ivanpauno.

Barring minor comments, I'm happy with it if CI's happy with it.

launch/launch/actions/execute_process.py

launch_pytest/launch_pytest/actions/__init__.py

launch_pytest/launch_pytest/fixture.py

launch_pytest/launch_pytest/plugin.py

launch_pytest/launch_pytest/tools/process.py

ivanpauno · 2021-10-26T14:33:44Z

@hidmic I have addressed you last comments.
I have also added a lot of test cases in 57656fe, 5dfb905, using the pytester plugin.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2021-10-29T14:54:58Z

Force pushed to resolve conflicts after #454 was merged.

This reverts commit af0c896. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2021-10-29T18:55:28Z

Windows is passing after 6c4f4d8 and 0279cf7:

Windows

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2021-10-29T20:30:37Z

@hidmic could you take another look?
I have fixed the windows issues mentioned above and I've also introduced compatibility with the version of pytest shipped in Focal, see a9300bf (we're using the pip install version in CI).

ivanpauno · 2021-10-29T20:32:08Z

CI:

Linux
Linux-aarch64
macOS
Windows (unrelated failures)

launch_pytest/test/launch_pytest/tools/test_process.py

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ros-discourse · 2021-11-01T20:52:31Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-10-28/22947/1

Blast545 · 2021-11-03T17:40:55Z

@ivanpauno Can I ask you to take a look to this PR? it introduced some regressions to the repeated jobs of the buildfarm:

nightly_linux_repeated#2459
nightly_linux-aarch64_repeated#1766/
nightly_win_rep#2416
nightly_osx_repeated#2526

I imagine it's missing a cleanup that's only showing once the tests are run in a sequence.

ivanpauno · 2021-11-03T18:58:09Z

@ivanpauno Can I ask you to take a look to this PR? it introduced some regressions to the repeated jobs of the buildfarm:

Interesting... I will take a look

ros-discourse · 2021-11-22T22:12:16Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-11-18/23209/1

wep21 · 2021-12-02T05:02:39Z

@ivanpauno @hidmic Is backporting launch_pytest to galactic acceptable? If so, I would like to create a backport PR for launch_pytest.

ivanpauno · 2022-01-05T13:06:10Z

@ivanpauno @hidmic Is backporting launch_pytest to galactic acceptable? If so, I would like to create a backport PR for launch_pytest.

I think that up to now it hasn't been used enough to say it's worth backporting.
I would rather start using it more and prove its usefulness before backporting it.

ivanpauno added the enhancement New feature or request label Sep 7, 2021

ivanpauno requested a review from hidmic September 7, 2021 19:09

ivanpauno self-assigned this Sep 7, 2021

hidmic reviewed Sep 7, 2021

View reviewed changes

ivanpauno requested review from wjwwood and jacobperron September 7, 2021 20:12

wjwwood reviewed Sep 7, 2021

View reviewed changes

launch/launch/launch_service.py Outdated Show resolved Hide resolved

launch_testing/launch_testing/pytest/plugin.py Outdated Show resolved Hide resolved

launch_testing/test/launch_testing/new_hooks/test_function_scope.py Outdated Show resolved Hide resolved

ivanpauno commented Sep 8, 2021

View reviewed changes

launch_testing/launch_testing/pytest/plugin.py Outdated Show resolved Hide resolved

ivanpauno mentioned this pull request Sep 9, 2021

Add python3-pytest-asyncio to rosdep ros/rosdistro#30756

Merged

ivanpauno requested a review from hidmic September 9, 2021 19:58

ivanpauno commented Sep 9, 2021

View reviewed changes

launch_testing/launch_testing/pytest/plugin.py Outdated Show resolved Hide resolved

hidmic reviewed Sep 9, 2021

View reviewed changes

ivanpauno requested a review from hidmic September 27, 2021 17:35

hidmic reviewed Oct 4, 2021

View reviewed changes

ivanpauno force-pushed the ivanpauno/launch-testing-pytest-prototype branch from 332ddf2 to 46ae686 Compare October 14, 2021 20:41

hidmic mentioned this pull request Oct 19, 2021

launch_testing @post_shutdown_test() checking output and return codes is flaky #534

Open

ivanpauno requested a review from hidmic October 19, 2021 21:05

hidmic approved these changes Oct 25, 2021

View reviewed changes

ivanpauno and others added 7 commits October 29, 2021 11:51

please linters

510d6d2

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Add a readme

5a593e7

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

fix linters

8de34a0

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Address peer review comments

0bb5147

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Improve code attribution

1b0096f

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

nit

5bd6f32

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

logs to debug on windows without a VM

af0c896

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/launch-testing-pytest-prototype branch from 74bbec3 to af0c896 Compare October 29, 2021 14:54

ivanpauno added 3 commits October 29, 2021 13:36

Revert "logs to debug on windows without a VM"

ee47439

This reverts commit af0c896. Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

fix windows (hopefully)

6c4f4d8

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

fix windows

0279cf7

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

compatibility with apt provided pytest

a9300bf

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno requested a review from hidmic October 29, 2021 20:30

hidmic approved these changes Nov 1, 2021

View reviewed changes

launch_pytest/test/launch_pytest/tools/test_process.py Outdated Show resolved Hide resolved

address peer review comment

cf66f4b

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno merged commit edcca09 into master Nov 1, 2021

delete-merged-branch bot deleted the ivanpauno/launch-testing-pytest-prototype branch November 1, 2021 18:07

ivanpauno mentioned this pull request Nov 3, 2021

* [launch_pytest] Fix issue when colcon --retest-until-fail flag is used #552

Merged

clalancette mentioned this pull request Apr 29, 2022

ROS 2 Humble needed release notes ros2/ros2_documentation#2446

Closed

87 tasks

takayuki5168 mentioned this pull request Jan 18, 2023

trajectory_follower_node smoke test failure autowarefoundation/autoware.universe#2684

Closed

3 tasks

First prototype of native pytest plugin for launch based tests #528

First prototype of native pytest plugin for launch based tests #528

Conversation

ivanpauno commented Sep 7, 2021

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpauno Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

hidmic commented Sep 7, 2021

ivanpauno commented Sep 8, 2021

hidmic commented Sep 8, 2021 • edited Loading

ivanpauno commented Sep 9, 2021

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Sep 14, 2021

ros-discourse commented Sep 20, 2021

wjwwood commented Sep 24, 2021

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpauno commented Oct 14, 2021 • edited Loading

ivanpauno commented Oct 19, 2021

ivanpauno commented Oct 19, 2021

ivanpauno commented Oct 19, 2021

hidmic left a comment

Choose a reason for hiding this comment

ivanpauno commented Oct 26, 2021

ivanpauno commented Oct 29, 2021

ivanpauno commented Oct 29, 2021 • edited Loading

ivanpauno commented Oct 29, 2021

ivanpauno commented Oct 29, 2021 • edited Loading

ros-discourse commented Nov 1, 2021

Blast545 commented Nov 3, 2021

ivanpauno commented Nov 3, 2021

ros-discourse commented Nov 22, 2021

wep21 commented Dec 2, 2021

ivanpauno commented Jan 5, 2022

ivanpauno Sep 8, 2021 •

edited

Loading

hidmic commented Sep 8, 2021 •

edited

Loading

ivanpauno commented Oct 14, 2021 •

edited

Loading

ivanpauno commented Oct 29, 2021 •

edited

Loading

ivanpauno commented Oct 29, 2021 •

edited

Loading