-
Notifications
You must be signed in to change notification settings - Fork 158
* [launch_pytest] Fix issue when colcon --retest-until-fail flag is used #552
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_pytest] Fix issue when colcon --retest-until-fail flag is used #552
Conversation
…sed. * [launch_pytest] Test all the examples. Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[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.
LGTM after green CI
# this function can use assertions to validate the output or return a boolean. | ||
# pytest generates easier to understand failures when assertions are used. | ||
assert output == 'hello_world\n', 'process never printed hello_world' | ||
assert process_tools.wait_for_output_sync( |
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.
Curious question, why was this assert removed?
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.
The function always returns None
in this case, though I think that's confusing.
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.
See a9cc81c
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.
The function always returns None in this case, though I think that's confusing.
It was not only confusing, it was also blocking unnecessarely untile the timeout expired.
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.
@ivanpauno meta: we clearly need a less terse idiom to write predicates. If we were to assume the predicate passed if it returns None
, that'd be practical yet surprising. If we were to simply return the result of evaluating the expression, we'd lose detailed assertion message (most of which are provided pytest
rewriting logic). So how about splitting use cases? E.g.:
# Wait for incoming process output with a timeout
output = wait_for_output_sync(process, timeout=10)
output = await wait_for_output(process, timeout=10)
# Wait for incoming process output matching a predicate with a timeout
def predicate(output):
return 'foo' in output
output = wait_for_output_sync(process, predicate, timeout=10)
output = await wait_for_output(process, predicate, timeout=10)
# Assert a statement on process output with a timeout
def statement(output):
assert output == 'foo'
assert_output_sync(process, statement, timeout=10)
await assert_output(process, statement, timeout=10)
In this case, it would read:
def hello_world_found():
assert output.splitlines() == ['hello_world'], 'process never printed hello_world'
assert_output_sync(launch_context, hello_world_proc, hello_world_found, timeout=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.
That sounds like the right approach to me.
I will address it in a follow-up PR so we solve the CI issue.
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.
Agreed, merge!
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[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.
LGTM but for one meta comment
# this function can use assertions to validate the output or return a boolean. | ||
# pytest generates easier to understand failures when assertions are used. | ||
assert output == 'hello_world\n', 'process never printed hello_world' | ||
assert process_tools.wait_for_output_sync( |
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.
@ivanpauno meta: we clearly need a less terse idiom to write predicates. If we were to assume the predicate passed if it returns None
, that'd be practical yet surprising. If we were to simply return the result of evaluating the expression, we'd lose detailed assertion message (most of which are provided pytest
rewriting logic). So how about splitting use cases? E.g.:
# Wait for incoming process output with a timeout
output = wait_for_output_sync(process, timeout=10)
output = await wait_for_output(process, timeout=10)
# Wait for incoming process output matching a predicate with a timeout
def predicate(output):
return 'foo' in output
output = wait_for_output_sync(process, predicate, timeout=10)
output = await wait_for_output(process, predicate, timeout=10)
# Assert a statement on process output with a timeout
def statement(output):
assert output == 'foo'
assert_output_sync(process, statement, timeout=10)
await assert_output(process, statement, timeout=10)
In this case, it would read:
def hello_world_found():
assert output.splitlines() == ['hello_world'], 'process never printed hello_world'
assert_output_sync(launch_context, hello_world_proc, hello_world_found, timeout=5)
Fixes #528 (comment).
test_module_scope.py / test_function_scope.py don't work when
--count=N
is passed, because now the test order is different.I moved them to the example folder and I'm now running then with the pytester plugin to avoid the issue.
I'm also running the other examples we have in that folder, ignoring one that would introduce a dependency on launch_ros (we have to move that one somewhere else later).
I've also fixed some issues in the existing examples.