Skip to content
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

Get some basic tests running #10

Merged
merged 15 commits into from Oct 1, 2019
Merged

Get some basic tests running #10

merged 15 commits into from Oct 1, 2019

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Jun 24, 2019

No description provided.

@cottsay cottsay added enhancement New feature or request in review Waiting for review (Kanban column) labels Jun 24, 2019
@cottsay cottsay requested a review from clalancette June 24, 2019 22:18
@cottsay cottsay self-assigned this Jun 24, 2019
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few things I don't understand in this PR:

  1. It does a bunch of work to change test_joy_twist.py, but that test never actually gets run by the CMakeLists.txt. Should we make it run in there?
  2. In my local testing, flake8 isn't happy with test_joy_twist.py; it is mostly complaining about import statements being in the wrong order and the use of double-quotes. I think we'll want to fix those before merging.
  3. There are a bunch of changes to the copyright statement at the top of test_joy_twist.py, but then the copyright check is essentially disabled in CMakeLists.txt. Either we should make the code changes to make the copyright check pass (and re-enable the test), or we should just disable that test (and thus we don't need the changes to the copyright). I'd prefer the former if we can make it happen, but the latter is acceptable as well.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Member Author

cottsay commented Jun 25, 2019

Thanks for taking a look, Chris.

  1. It isn't being used. I ported that script to ROS 2 on a functional level, but I'm not sure how to integrate it. Guidance on the path forward here would be appreciated.
  2. Sorry about that - you probably have a different flake8 than I do. I sorted the imports and switched to single quotes. (It did pass on my machine and on the buildfarm)
  3. I changed the copyright because the lines were too long and it was upsetting flake8. The rest of the files are the reason I didn't enable that test. Adding copyrights seems like something we should do in ROS 1 as well.

@clalancette
Copy link

1. It isn't being used. I ported that script to ROS 2 on a functional level, but I'm not sure how to integrate it. Guidance on the path forward here would be appreciated.

Ah, I see. The major thing wrong with that test as it stands is that it does not use the launch_testing infrastructure. In order for this test to work, it would need to bring that stuff in and then launch the unittest alongside an invocation of teleop_twist_joy. One other thing to keep in mind if you go looking at this is that the "axes" field of the message must have the dead-man button enabled (button 5 by default), otherwise nothing will come out of teleop_twist_joy. Once you have those things in place, I think this test would actually work.

2. Sorry about that - you probably have a different flake8 than I do. I sorted the imports and switched to single quotes. (It did pass on my machine and on the buildfarm)

This is mostly fixed, though my flake8 was still complaining about the order of includes; basically "time" and "unittest" need to be before everything else.

3. I changed the copyright because the lines were too long and it was upsetting flake8. The rest of the files are the reason I didn't enable that test. Adding copyrights seems like something we should do in ROS 1 as well.

All right, makes sense. I'm fine with that change to make flake happy. I agree about adding the copyrights to ROS 1, but it honestly isn't that urgent.

CMakeLists.txt Outdated Show resolved Hide resolved
cottsay and others added 4 commits September 30, 2019 14:25
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made one small change, in that I removed the shebang from test_joy_twist.py; it's always imported and used, never run directly. Other than that, it looks good to me so I'll merge this. Thanks for iterating.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette merged commit 1b379f5 into dashing Oct 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the tests branch October 1, 2019 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants