Conversation
|
This does not add any test yet only an executable. |
|
Alright, now I have actually added the nosetest in ament. |
There was a problem hiding this comment.
Why all this copied complexity if the test is not actually ever running a publisher or receives a message?
|
I reduced the test quite a lot since I probably won't use the extra functionality copied from |
There was a problem hiding this comment.
Uninitialized variable. Also avoiding global variables would be better.
There was a problem hiding this comment.
Can I just throw a runtime error in the callback instead, since it's not expecting to receive a message?
|
This should be moved to the |
|
should I convert this test to a gtest to be consistent with the other test in |
|
I would say keeping it as a nosetest is perfectly fine. |
2787a73 to
abbac31
Compare
|
After looking at it again I have to change my opinion: since the test only uses a single executable a GTest would be less overhead then a nosetest with a launch file. |
|
+1 |
1 similar comment
|
+1 |
|
Since there's not multi-node testing a gtest would be better. It's much less overhead setting up and can support much more precise testing. |
|
@jacquelinekay You have merged the other related PRs but not this one. Currently all platforms fail to build: Please fix it. |
8a4e516 to
e868e4c
Compare
|
I've converted the test to a Gtest and rebased. This pull request will fix the broken build, so assuming @dirk-thomas and @tfoote find these changes acceptable, I will squash and merge after CI comes out clean. Sound good? http://ci.ros2.org/job/ros2_batch_ci_linux/149/ |
|
The commit log and diff contains entries which are already on master. So something is wrong with the rebase of this branch. |
|
The Windows build failed with: May be @j-rivero is currently installing the released version of VS? |
|
Ouch yes, sorry, I started but did not finish the process, mainly because of this question. Edit: by the way, May I have rights (I already logged in using the github account) in jenkins so I can disable the node temporary while I'm updating the system? Thanks. |
e868e4c to
4087630
Compare
|
Did not pull before rebasing, should look better now. |
There was a problem hiding this comment.
Please don't change this. You likely don't have an up-to-date version of the ament linter repo.
|
|
Without the definition the three result files will have the same name and therefore don't tell the user which exact test is running. Leaving it out won't affect the result of the build - only the generated test result files. You can keep the current CI jobs running. We still need to merge this asap since currently nobody can build the master branches. As soon as the Windows build works again you should check for regressions and address them after the fact. |
|
I will relaunch the job and post the results or feedback here as soon as the windows node is back. |
239a9eb to
00dcf2c
Compare
|
Linux and OSX are clean. I'm merging. |
Add test for timeout parameter
Connects to ros2/ros2#73
This test could be prone to flakiness due to scheduling nondeterminism, hence the extremely high tolerance. If we'd prefer to avoid flaky tests, I'm fine with declining this change, but I posted it to prove the correctness of my timeout PRs.