-
Notifications
You must be signed in to change notification settings - Fork 144
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
[launch_testing] remove deprecated ready_fn
feature
#589
Conversation
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Should this line be removed as well ?
|
The short answer is: "I don't know" I looked at that line, but I couldn't figure out if it is part of the test runner machinery that is unfortunately named the same as the old feature users would use, or not. That's why I ran CI, I figured that if it was the old feature I'm removing some tests should fail, but maybe that's not the case. I also sort of traced the logic and I believe that it is still used, but the difference is that the Maybe someone more familiar with |
@adityapande-1995 @wjwwood It shouldn't. That's the actual callable that the
TBH this whole thing is messy. |
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.
@wjwwood LGTM but
launch/launch_testing/launch_testing/test_runner.py
Lines 295 to 296 in b59bb6b
if argname == 'ready_fn': | |
continue |
should be removed too.
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood linters were still failing in that last CI. |
Signed-off-by: William Woodall <william@osrfoundation.org>
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!
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.
Looks good to me!
This feature was deprecated in #346 (foxy) and should be removed by now, so we'll do it in Humble.
We realized this due to this pr (thanks for that btw): #580
I'm opening in draft because I quickly did this in the browser and I'm not sure if the relatedReadyAggregator
class needs to be removed. So I'll let CI run first.CI is good, but I'm still not sure about the
ReadyAggregator
class, looking for input on that.