Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrates legacy launch API tests #167

Merged
merged 17 commits into from
Feb 27, 2019
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 29, 2019

Connected to #159. This pull request adds the bare minimum functionality to migrate all tests in ROS 2 core packages that make use of the legacy launch API. May eventually face deprecation depending on the path taken after #165.

Some launch.legacy tests still remain to be ported, unclear if these should in the first place as such additions may as well fall under the improving test coverage for launch umbrella.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Jan 29, 2019
@hidmic hidmic added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 30, 2019
@hidmic
Copy link
Contributor Author

hidmic commented Jan 31, 2019

I think it makes sense to get rid of the legacy API regardless of what comes out of the launch based testing framework work. Unrelated functionality I had to add I think could still be useful outside testing, do tell if you think otherwise.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good to me so far, though I had a some questions during review.

launch/launch/events/__init__.py Outdated Show resolved Hide resolved
launch_testing/launch_testing/__init__.py Outdated Show resolved Hide resolved
launch/launch/action.py Outdated Show resolved Hide resolved
launch/launch/action.py Outdated Show resolved Hide resolved
launch_testing/launch_testing/__init__.py Outdated Show resolved Hide resolved
if all(status == 'succeeded' for status in self.__tests.values()):
return EmitEvent(event=Shutdown(reason='all tests finished'))

def add_fixture_action(self, launch_description, action, required=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the launch_description in here was a bit off-putting to me at first, but I see the reason for it now, though I wish it were more transparent what was happening. If you look just at the calling code (like at one of the converted tests), you don't get a since as to what is being done to the launch description, you can assume it's adding the fixture/test/output_test or whatever, but I hadn't imagined it would add more actions until reading the code for it here.

A possible alternative would be to make this function simply generate and return a list of actions to be added, rather than adding them directly. Though I'm not sure if the launch description is required within this function in any other cases. For example, it might look like:

my_test_exec_action = ExecuteProcess(...)
fixture_action = lts.create_fixture_action(my_test_exec_action)
ld.add_action(fixture_action)

In that example fixture_action might be just the original action or a GroupAction that consists of several things.

I'm not 100% sold on my approach either, as I wouldn't be able to know without an example or very good docs if I should add the original action or just the result of create_fixture_action() instead.

Copy link
Contributor Author

@hidmic hidmic Feb 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the launch_description in here was a bit off-putting to me at first...

It sort of is to me as well, but as I said before, I was trying to minimize the amount of code changes (because it's not at all unlikely we end up ditching this in the near future) and thus I kept the API the way it was.

A possible alternative would be to make this function simply generate and return a list of actions to be added, rather than adding them directly.

That is an option, sure. Another easy one (easy to use I mean) would be to have a LaunchTestDescription along with the LaunchTestService and make them wrap the LaunchDescription and LaunchService respectively. Not sure if it makes sense to spend time polishing this API though.

wjwwood and others added 2 commits February 4, 2019 10:50
Co-Authored-By: hidmic <michel@ekumenlabs.com>
- Emit ExecutionComplete events on demand.
- Shutdown OpaqueCoroutine gracefully.
- Fix failing test cases.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 4, 2019

Running CI for all related PRs using pinned FastRTPS:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux packaging Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno mentioned this pull request Feb 7, 2019
To cope with more test cases.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 12, 2019

@mjcarroll @sloretz @wjwwood I'm a bit unsure on how to handle bad exit codes upon shutdown in a true cross platform way, thus I'm blatantly ignoring them. Not very proud, but I can't figure out how to do better than that.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/drop-legacy-launch branch 2 times, most recently from d26d890 to ea034a2 Compare February 18, 2019 21:00
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 20, 2019

So, we have a problem with FastRTPS talking to Opensplice and Connext on Windows. Looks like we have an issue tracking it (and someone suggesting that FastRTPS v.1.7.1 may be the cure). But since this predates these changes (which have only revealed more issues), in the interest of time I'd say: let's disable the offending test just as we did before, and follow up that issue. We can then come back and re-enable all tests.

@mjcarroll @sloretz @ivanpauno thoughts?

@cottsay cottsay requested review from mjcarroll and removed request for mjcarroll February 22, 2019 19:37
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 25, 2019

CI is green! @mjcarroll mind to take a shot at this? We can re-run CI on all platforms right after.

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some tiny documentation nits, but otherwise LGTM.

.. code-block:: python

async def coroutine(
context: LaunchContext, # iff ignore_context is False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what iff is in this context, but it may be helpful to be a little bit more explicit. It may also be worth updating the docs for OpaqueFunction so that they are consistent in this regard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I've updated the documentation, but I'm not sure what you mean about changing OpaqueFunction docs.

super().__init__(**left_over_kwargs)
if not asyncio.iscoroutinefunction(coroutine):
raise TypeError(
"OpaqueCoroutine expected a couroutine for 'couroutine', got '{}'".format(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couroutine -> coroutine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch!

"""
Convenience class for handling an action completion event.

It may be configured to only handle the completion of a specific action,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an example or test for handling specific vs all? I can't seem to find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we do not. Test added.

- Improved OpaqueCoroutine documentation.
- Added test for launch.event_handlers.OnExecutionComplete.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 25, 2019

Grrr something changed on actions and sros2, tests are failing again.

@hidmic
Copy link
Contributor Author

hidmic commented Feb 26, 2019

@mjcarroll CI got unstable because (a) I had to rebase ros2/system_tests branch after ros2/system_tests#330 and (b) there's a test failure coming from sros2 and it's not specific to this set of PRs (see https://ci.ros2.org/job/ci_osx/5150/ or https://ci.ros2.org/job/ci_linux/6268/). So if we run CI again, it'll go yellow again.

We can wait till (b) gets fixed, but I'm afraid more things will get in the way after that. It's fine by me, but that means more delay for these PRs.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 6, 2019

@mjcarroll could you take a look at the dependency fixing PRs?

@mjcarroll
Copy link
Member

Done!

@hidmic hidmic deleted the hidmic/drop-legacy-launch branch March 6, 2019 17:04
This was referenced Mar 7, 2019
@wjwwood
Copy link
Member

wjwwood commented Mar 27, 2019

There was a bug fix in this pr that was unrelated to the original change. @stonier opened a pr for just this change here:

#220

I declined it so that we could put it through our typical patch release process.

The change is small, @nuclearsandwich FYI.

@wjwwood wjwwood mentioned this pull request Mar 27, 2019
8 tasks
@nuclearsandwich
Copy link
Member

I declined it so that we could put it through our typical patch release process.

The change is small, @nuclearsandwich FYI.

Thanks for the ping. I don't mind if the original PR gets re-opened or if we open a new PR explicitly preserving the original commit authors as long as we run Crystal CI on it before merging and releasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants