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.

Sign up for GitHub

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

Jump to bottom

Windows Fixes #87

Merged
merged 6 commits into from
Jun 21, 2018
Merged

Windows Fixes #87

merged 6 commits into from
Jun 21, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 20, 2018 •
edited
Loading

Fixes #86

The first commit (there will be more) should cause the tests to fail, where they were silently ignoring exceptions that made it fail on Windows.

@wjwwood wjwwood added bug Something isn't working in progress Actively being worked on (Kanban column) labels Jun 20, 2018
@wjwwood wjwwood added this to the bouncy milestone Jun 20, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 21, 2018

This is ready for review now (this is needed for Bouncy).

CI:

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

@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 21, 2018
dhood
dhood reviewed Jun 21, 2018
View reviewed changes
Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

this commit I'm not too sure on: e14e644 but otherwise it looks good. Tested on xenial and works as expected.

@dhood
Copy link
Member

dhood commented Jun 21, 2018

There's a traceback in the CI testing output: https://ci.ros2.org/job/ci_linux/4774/console#console-section-12

Task was destroyed but it is pending!
...
task: <Task pending coro=<LaunchContext.emit_event() running at /home/rosbuild/ci_scripts/ws/src/ros2/launch/launch/launch/launch_context.py:172> cb=[_chain_future.<locals>._call_set_state() at /usr/lib/python3.6/asyncio/futures.py:410] created at /usr/lib/python3.6/asyncio/tasks.py:710>
sys:1: RuntimeWarning: coroutine 'LaunchContext.emit_event' was never awaited

@wjwwood
Copy link
Member Author

wjwwood commented Jun 21, 2018

Oh good catch, I'll look into that.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 21, 2018

It only happens when you use the "repeat until fail" logic, which translates to --count N as a pytest argument (btw it's very frustrating that you cannot see the pytest invocation).

@wjwwood
Copy link
Member Author

wjwwood commented Jun 21, 2018

New CI:

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

@wjwwood wjwwood merged commit 5526541 into master Jun 21, 2018
@wjwwood wjwwood deleted the windows_fixup branch June 21, 2018 03:38
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 21, 2018
@dhood
Copy link
Member

dhood commented Jun 21, 2018

For future readers, it was caused by a race condition (wasn't specific to pytest)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhood dhood dhood approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
bouncy
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@wjwwood @dhood