-
Notifications
You must be signed in to change notification settings - Fork 168
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
Remove ready_fn, self.proc_info #376
Conversation
@pbaughman it looks like there's a merge conflict. Would you mind rebasing this one? |
@sloretz Yeah, I'll probably need to update some more tests, too. I think the MR causing the conflict added some new stuff as well |
169b864
to
c61418e
Compare
@pbaughman Looks like there are 4 test failures with the same message. Mind taking a look?
|
@sloretz Yeah - sorry - the required changes to launch_testing weren't in the nightly docker image yet, and then I put this down and forgot about it in the middle of adding launch to the workspace. I'll get it working first thing tomorrow |
@sloretz This looks like a bug/not implemented thing in launch.actions.ExecuteProcess. The ExecuteProcess action isn't returning any sub-entities, so launch_testing can't discover the ReadyToTest action. I'll need to address that in launch before we can move ahead with this. Edit ros2/launch#347 |
c61418e
to
e19e062
Compare
Looks like something is still a little wrong
|
1d6ff41
to
69d4e16
Compare
@sloretz I think this is good to go after rebasing. Can you run CI? I don't get any ros2topic failures when I run the tests locally on 'master' and on my branch. I do see failures in ros2actions but those happen for me in 'master' |
@sloretz Can you re-run CI on this? I don't see any failure in this branch that aren't already in 'master' |
@sloretz Sorry to keep bugging you, but poke ^^ |
69d4e16
to
002ef31
Compare
@sloretz Looks like something with the build failed?
It doesn't look like it even made it to the 'test' step I'm guessing these packages were renamed or something on 'master' I rebased the branch again |
Ah oops. I copy pasted the args I used last time, but it looks like those were deleted in #407. CI (Testing ros2action ros2interface ros2node ros2pkg ros2service ros2topic) |
@sloretz I'm pretty sure this is a bad test that tries to assert about the @hidmic Just FYI if you're seeing similar problems on other similar tests ^^ |
I'll need to fix ros2/launch#381 before I can fix this test. I want to add a stdout_ready_listener to delay the start of the test until the fibonacci_action_server.py process is started up, but I can't because event_handler introspection appears to be broken right now |
Waiting on ros2/launch#386 so I can delay the start of the tests until after the Fibonacci action server is up-and-running |
002ef31
to
d2eed85
Compare
d2eed85
to
3be5c40
Compare
@sloretz ping^^ |
This looks unrelated to my changes again. . .
I'll try rebasing again I guess. |
3be5c40
to
50f40fa
Compare
@sloretz Yeah, 96cf5b9 changed this yesterday and I pushed to this branch five days ago so it broke out from under me. Please run it again I guess. I'm going to be so happy when this PR is gone forever. Side topic - Should I just log in every day and rebase this branch while I'm waiting for someone to run CI? This is not the first time that it's gotten broken out from under me. How can I make faster progress here? |
CI (Testing ros2action ros2interface ros2node ros2pkg ros2service ros2topic)
Thank you for sticking with it!
Rebasing frequently helps. That works around ci.ros2.org being incapable of merging forks into the target branch and then building/testing. |
Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
50f40fa
to
dee3739
Compare
@sloretz Still looks like upstream issues with the build. It didn't even make it to the tests:
No idea what's been going on with sqllite3_vendor or rosbag2. Gonna rebase again? |
The CI jobs linked do not look like the correct ones. |
Good catch. It looks like the jobs were queue'd, but some earlier in the queue got aborted so the real job numbers didn't match. I've fixed the links in the comment above. @pbaughman FYI |
The failures using CycloneDDS is a known issue: #457 |
Connext failures also appear, sometimes, on the nightlies (for example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It looks like all the failures in ros2service.test.test_cli.test_cli, ros2node.test.test_cli.test_cli, and ros2action.test.test_cli.test_cli were cyclone DDS failures. The connext failures seem like they're only in ros2topic.test.test_cli.test_cli |
@sloretz I'll defer to you for merging this. |
Thanks for the PR and for sticking with it @pbaughman ! |
The ready_fn will be deprecated in the future in favor of the ReadyToTest() action in launch_testing. See ros2/launch#346 (comment) for background information
Signed-off-by: Pete Baughman pete.baughman@apex.ai