Added unit tests for the nodehandles #37
Conversation
|
What is holding back this PR from being integrated in the repository? |
|
Hi @progtologist, Having additional unit tests here is very valuable. Although, I'd prefer not adding known failing tests if we don't have a fix about to be merged. The main motivation for not merging known failing tests is that it will prevent us from detecting regressions in the future. One way to get this PR in would be to comment out the failing test and link to it in the appropriate issue. Then it can be reenabled to validate the corresponding fix when it's ready for review. |
|
Hello @mikaelarguedas, I never got the time to investigate the problem in roscpp, as far as I could tell, the problem was far from trivial and required some effort that I could not justify in my schedule. Also unit tests in roscpp are not really covering all possible combinations, so I wouldn't want to break stuff while fixing this issue. Since you wouldn't want to merge tests that fail without their respective fixes (justifiable), can you postpone closing this until I can workout a fix for the nodehandles (possibly on February or March)? |
|
@progtologist I'm considering commenting out the failing test and keep the issue open to track it but merge the other ones because they are great additions that improve coverage. Would you be ok with that for the time being ? |
|
@progtologist I created #64 rebasing this branch and commenting out the failing test. Once #7 has been fixed upstream, c88d78d can be reverted to reenable the test Thanks for the contribution! |
|
Closing in favor of #64 |
Added unit tests that check that the nodelet private and global nodehandles are working correctly. Provided three different launch files that test different scenarios. One of the unit tests is failing.