-
Notifications
You must be signed in to change notification settings - Fork 340
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
Add demos for using logger service #611
Add demos for using logger service #611
Conversation
Signed-off-by: Barry Xu <[email protected]>
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.
overall looks good to me, some changes are requested.
Signed-off-by: Barry Xu <[email protected]>
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.
some suggestions
Signed-off-by: Barry Xu <[email protected]>
I updated code according to your comments. |
@clalancette CI green, can you do final review? |
@@ -0,0 +1,18 @@ | |||
Output with default logger level: |
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.
I'm noticing that when running the test here, the test itself takes like 1 second to run, but then it sits around for an additional 5 seconds before the test completes. That means each invocation of this test takes at least 6 seconds, and across 4 RMWs (rmw_fastrtps_cpp
, rmw_fastrtps_dynamic_cpp
, rmw_cyclonedds_cpp
, and rmw_connextdds
), this will add at least 24 seconds to our CI times.
Do you mind taking a look here and seeing what is causing that extra 5 second delay?
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.
Not only did this test take an extra 5 seconds, but I also noticed that all the other tests took at least 5 seconds as well. So, the issue is likely within the testing framework itself.
Currently, I am not familiar with the current testing framework, so it may take some time to investigate
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.
Find the below code to sleep 5 seconds in test framework. I think we can reduce the sleep time.
demos/demo_nodes_cpp/test/test_executables_tutorial.py.in
Lines 66 to 70 in 74738d8
# TODO(hidmic): either make the underlying executables resilient to | |
# interruptions close/during shutdown OR adapt the testing suite to | |
# better cope with it. | |
import time | |
time.sleep(5) |
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.
@clalancette this is not really related to this PR, can we have another issue to follow-up? or if you have suggestions, can you share it here?
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.
@clalancette this is not really related to this PR, can we have another issue to follow-up? or if you have suggestions, can you share it here?
Yes, you are correct. If you could please open a separate issue to reduce this timeout in the framework, then I think we are fine to move forward here.
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.
If you could please open a separate issue to reduce this timeout in the framework, then I think we are fine to move forward here.
I created a new issue #628 to trace this problem. @clalancette
Signed-off-by: Barry Xu <[email protected]>
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.
One very minor thing to fix, then we'll be good to run another CI on this and merge.
Signed-off-by: Barry Xu <[email protected]>
Depend on