-
Notifications
You must be signed in to change notification settings - Fork 8
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
Feature/testing framework #41
Conversation
…o-express containers.
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.
Reviewed and tested. db.test worked well for me. Once you address the comments and add a little documentation, I think it'll be about ready after that (assuming the use case is to simply provide people a place to start--some discussion on use cases may be warranted).
<!-- This is an integrative test that requires the speaker interaction --> | ||
<include file="$(find harmoni_speaker)/launch/speaker_service.launch"></include> | ||
<include file="$(find harmoni_tts)/launch/tts_service.launch"></include> | ||
<include file="$(find harmoni_bot)/launch/bot_service.launch"></include> | ||
<include file="$(find harmoni_face)/launch/face_service.launch"></include> | ||
<include file="$(find harmoni_pattern)/launch/sequence_pattern.launch"> | ||
<arg name="pattern_name" default="speak_test"/> | ||
</include> | ||
|
||
|
||
<!-- <rosparam file="$(find harmoni_pattern)/config/configuration.yaml" subst_value="True"/> | ||
<param name="instance_id" value="default"/> | ||
<node pkg="harmoni_pattern" type="sequential_pattern.py" name="harmoni_pattern_default" output="screen"/> --> | ||
|
||
|
||
<test test-name="test_sequential" pkg="harmoni_pattern" type="test_sequential_speak.py"/> | ||
</launch> |
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 really like this integration test =) Provides a good example of using almost the entire system and could probably be referenced as part of a tutorial.
Since it requires user interaction, documentation is needed somewhere that specifies what people are expected to do.
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.
Agree. I have not checked Chris documentation yet.
@chrismbirmingham do you have any reference to this integration test?
def test_IO(self): | ||
rospy.loginfo( | ||
"TestSequential[TEST]: basic test to ensure pattern can start a service " | ||
) | ||
while not rospy.is_shutdown() and not self.result: | ||
self.rate.sleep() | ||
assert self.result == True |
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.
Need to add more test cases before we can say this is complete, right? At least some quantitative indication that the sequence is moved through correctly.
Note: the setUp and init functions will be re-run for each test case...this is usually okay, but if you don't like it, or need to put all nodes into a specific state afterwards, I recommend using setUpClass (uses class variables instead of class instance variables) and/or tearDown().
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. Let me add some indicators about the progress of the testing.
As regards your note, I think that for now we can keep the setUp and init funcion that run every time since we have a single testing function.
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.
Normally you would want to separate out your test functions so that you know which one passes and which one does not...I suppose since it's not automated and requires user interaction though, you can just do a single test function and print out statements based on progress through the test.
<node pkg="harmoni_pattern" type="sequential_pattern.py" name="harmoni_pattern_default" output="screen"/> --> | ||
|
||
|
||
<test test-name="test_sequential" pkg="harmoni_pattern" type="test_sequential_speak.py"/> |
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.
Should specify time-limit="###.#"
since you likely need more than 60 seconds to test this.
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.
Thank you! I'm adding it for this testing
@@ -0,0 +1,53 @@ | |||
#!/usr/bin/env python3 |
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.
Calling this a unit test would indicate it can be tested without roscore running (i.e. just like a regular python package)...is this true?
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.
It was supposed to be an unittest, but it is not yet.
harmoni_core/harmoni_recorder/src/harmoni_recorder/template_storedata.py
Show resolved
Hide resolved
@@ -0,0 +1 @@ | |||
pymongo |
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 added this file, but we should update the relevant dockerfiles to add this dependency too.
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
…ction-lab/HARMONI into feature/testing-framework
Co-authored-by: Michael Swan <[email protected]>
@@ -30,12 +30,13 @@ class TestSequential(unittest.TestCase): | |||
|
|||
def __init__(self, *args): | |||
super(TestSequential, self).__init__(*args) |
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 don't define init at all, the super init call will automatically occur.
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.
Do you suggest to remove the init in all the testing files for all the packages?
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.
Sure. Worked fine for my testing, and consistency is good.
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.
Done it
This is a sequence of: | ||
- bot | ||
- tts | ||
- speak and lip-sync | ||
If all the requests succeed, the test passes. |
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 suppose it's okay for now, but long term I would want to make sure it runs correctly for any possible sequential sequence, not just testing some specific sequence.
Merging testing branch with develop