Cpp tests #64

ernestmc · 2016-01-12T21:27:39Z

dirk-thomas · 2016-01-13T17:35:25Z

rosidl_generator_cpp/test/test_array_generator.hpp

+template<std::size_t SIZE>
+void test_array_bool(std::array<bool, SIZE> * dst_array)
+{
+  int i;


Please declare the variable in the smallest scope possible (https://github.com/ros2/ros2/wiki/Developer-Guide#programming-conventions). In this case within the for loop.

Same below.

ernestmc · 2016-01-26T21:01:00Z

This is ready for review.

Added tests for the following types:

primitives_static
primitives_bounded
primitives_unbounded
static_array_bounded
static_array_unbounded
static_array_static
bounded_array_bounded
bounded_array_unbounded
bounded_array_static
unbounded_array_bounded
unbounded_array_unbounded
unbounded_array_static

$ valgrind --leak-check=yes ./test_interfaces_cpp
==21088== Memcheck, a memory error detector
==21088== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==21088== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==21088== Command: ./test_interfaces_cpp
==21088== 
Running main() from gtest_main.cc
[==========] Running 13 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from Test_rosidl_generator_traits
[ RUN      ] Test_rosidl_generator_traits.has_fixed_size
[       OK ] Test_rosidl_generator_traits.has_fixed_size (10 ms)
[----------] 1 test from Test_rosidl_generator_traits (22 ms total)

[----------] 12 tests from Test_messages
[ RUN      ] Test_messages.primitives_static
[       OK ] Test_messages.primitives_static (22 ms)
[ RUN      ] Test_messages.primitives_bounded
[       OK ] Test_messages.primitives_bounded (124 ms)
[ RUN      ] Test_messages.primitives_unbounded
[       OK ] Test_messages.primitives_unbounded (259 ms)
[ RUN      ] Test_messages.static_array_bounded
[       OK ] Test_messages.static_array_bounded (25 ms)
[ RUN      ] Test_messages.static_array_unbounded
[       OK ] Test_messages.static_array_unbounded (38 ms)
[ RUN      ] Test_messages.static_array_static
[       OK ] Test_messages.static_array_static (24 ms)
[ RUN      ] Test_messages.bounded_array_bounded
[       OK ] Test_messages.bounded_array_bounded (24 ms)
[ RUN      ] Test_messages.bounded_array_unbounded
[       OK ] Test_messages.bounded_array_unbounded (37 ms)
[ RUN      ] Test_messages.bounded_array_static
[       OK ] Test_messages.bounded_array_static (24 ms)
[ RUN      ] Test_messages.unbounded_array_bounded
[       OK ] Test_messages.unbounded_array_bounded (35 ms)
[ RUN      ] Test_messages.unbounded_array_unbounded
[       OK ] Test_messages.unbounded_array_unbounded (51 ms)
[ RUN      ] Test_messages.unbounded_array_static
[       OK ] Test_messages.unbounded_array_static (37 ms)
[----------] 12 tests from Test_messages (703 ms total)

[----------] Global test environment tear-down
[==========] 13 tests from 2 test cases ran. (774 ms total)
[  PASSED  ] 13 tests.
==21088== 
==21088== HEAP SUMMARY:
==21088==     in use at exit: 0 bytes in 0 blocks
==21088==   total heap usage: 609 allocs, 609 frees, 76,164 bytes allocated
==21088== 
==21088== All heap blocks were freed -- no leaks are possible
==21088== 
==21088== For counts of detected and suppressed errors, rerun with: -v
==21088== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

jacquelinekay · 2016-01-27T22:47:58Z

rosidl_generator_cpp/test/test_array_generator.hpp

+
+/**
+ * Helper function to generate a test pattern for boolean type.
+ * Alternating true (even index) and false (odd index) pattern.


If you're doing Doxygen-style comments, can you \param tags for each parameter

* \param[in] container (comment about parameter...) * \param[in] size (comment about parameter...

Applies below too.

Added Doxygen comments.

jacquelinekay · 2016-01-28T23:10:46Z

Good stuff overall, especially the random pattern generation (I think the fancy word for that is "fuzz testing") and using the MAX and MIN values for datatypes. I'll probably incorporate that into my tests for freertps serialization on my next pass of that.

My major feedback about this PR is what I said about consolidating the code with templates. Feel free to email me if you want more direction on how to do that.

ernestmc · 2016-01-29T17:13:12Z

@jacquelinekay thanks for your comments. I'll be addressing them soon.

jacquelinekay · 2016-02-03T00:23:21Z

@ernestmc So sorry for leading you down the wrong path--I looked a bit closer at the code and realized I should have said for you to use macros, not templates.

Check out this commit, to branch cpp_tests_macros:

240b28e

If you think you understand that example, go ahead and try simplifying the rest of this test case with macros. Otherwise let me know and I can answer your questions, or finish it in that branch.

jacquelinekay · 2016-02-03T00:24:00Z

@dirk-thomas or someone else may have strong feelings about where those testing macros are defined--usually macros should go in a separate header file, or at the top of the file.

ernestmc · 2016-02-03T21:38:06Z

Thanks @jacquelinekay ! I managed to templatize the test pattern generator functions using type traits, so now one function name works for all. Now I'll write the macros following your model. The code should be much nicer now.

ernestmc · 2016-02-04T17:42:05Z

Updated and putting for review.
The code is much nicer and readable. Templatized the test pattern generation functions, added macros and functions to take away the repeated code of the tests.

jacquelinekay · 2016-02-04T17:45:20Z

Looks great!

I started CI jobs:
http://ci.ros2.org/job/ci_linux/894/
http://ci.ros2.org/job/ci_osx/749
http://ci.ros2.org/job/ci_windows/975

If those come out ok, I think we should merge this.

jacquelinekay · 2016-02-04T18:12:04Z

The cpplint style checker tool on Jenkins wants test_interfaces.cpp to include vector:

http://ci.ros2.org/job/ci_linux/894/testReport/rosidl_generator_cpp/cpplint/build_include_what_you_use__4____home_rosbuild_ci_scripts_ws_src_ros2_rosidl_rosidl_generator_cpp_test_test_interfaces_cpp_176_/

ernestmc · 2016-02-04T18:39:26Z

Good advice hehe. How can I add this linter so it runs when I build with ament?

jacquelinekay · 2016-02-04T18:44:22Z

I have 2 ways of doing this locally:

source the install/setup.bash made by ament, then the command ament_cpplint will be on your PATH. You can call it on a folder or an individual file.
Run:
/ament test --skip-build --skip-install --only rosidl_generator_cpp
You can omit the first path if your install/setup.bash is sourced.
This will run the entire test suite on the package in your workspace. The test suite includes all the linters we use (cpplint, uncrustify, cppcheck, etc.)
Of course, you can use different flags, e.g. if you want to rebuild and reinstall, omit "skip-build" and "skip-install", and if you want to test the entire workspace, don't use "only".

ernestmc · 2016-02-04T18:54:59Z

I use $ ament test src/ros2/rosidl/rosidl_generator_cpp
and it runs 7 different tests:

test_interfaces_cpp
copyright
cppcheck
lint_cmake
pep8
pyflakes
uncrustify

jacquelinekay · 2016-02-04T19:00:30Z

Interesting, I wonder why cpplint didn't run locally for you.

Maybe cpplint was added to the set of linters for this package recently and you need to rebase against master?

@dirk-thomas would you like Ernesto to squash this? I think it's ready to merge.

jacquelinekay · 2016-02-04T19:16:07Z

ah, my bad, I didn't look at the status of the Windows build.

http://ci.ros2.org/job/ci_windows/975/

Sometimes getting aggressive with templates will cause Visual Studios to fail.

17:51:49   C:\J\workspace\ci_windows\ws\src\ros2\rosidl\rosidl_generator_cpp\test\test_interfaces.cpp(145): error C2668: 'test_vector_fill': ambiguous call to overloaded function [C:\J\workspace\ci_windows\ws\build\rosidl_generator_cpp\test_interfaces_cpp.vcxproj]
17:51:49   C:\J\workspace\ci_windows\ws\src\ros2\rosidl\rosidl_generator_cpp\test\test_interfaces.cpp(186): error C2668: 'test_vector_fill': ambiguous call to overloaded function [C:\J\workspace\ci_windows\ws\build\rosidl_generator_cpp\test_interfaces_cpp.vcxproj]

I will investigate this since I have a Windows VM set up.

ernestmc · 2016-02-04T19:30:51Z

hmmm I think that could be happening because a bool type is also an integral type and both templates could be getting enabled. I'll try adding an extra check (not sure if it's possible)

ernestmc · 2016-02-05T13:46:12Z

@dirk-thomas I had to add <test_depend>ament_cpplint</test_depend> to package.xml

ernestmc · 2016-02-05T13:57:53Z

@esteve could you test my last changes on Windows?

dirk-thomas · 2016-02-05T17:06:03Z

Which package.xml file are you referring to? This PR doesn't contain any changes and the manifest already contains all common linters: https://github.com/ros2/rosidl/blob/master/rosidl_generator_cpp/package.xml#L21

ernestmc · 2016-02-05T17:18:38Z

@dirk-thomas I was referring to precisely that file. I added the changes locally to test. Although lint_common is present for some reason it doesn't invoke cpplint unless I explicitly add it.

jacquelinekay · 2016-02-05T17:30:27Z

It's because that test dependency was added to the package.xml after you branched this branch off of master, and you need to either merge or rebase on the master branch.

esteve · 2016-02-05T17:33:11Z

@ernestmc running it now, cross your fingers! 😉

dirk-thomas · 2016-02-05T17:33:57Z

ament_lint_common (https://github.com/ament/ament_lint/blob/master/ament_lint_common/package.xml) does already include ament_cmake_cpplint. Can you check the content of the file locally? Are you having the latest version of the ament_lint repo?

ernestmc · 2016-02-05T17:39:41Z

@dirk-thomas I guess I don't have the latest version of the ament_lint repo because ament_cpplint and ament_clang_format are commented out.

esteve · 2016-02-05T17:54:05Z

@ernestmc yay, it worked! Thank you so much for your patience going through all our feedback. The current PR looks good to me and ready to be merged. Could you squash the commits and trigger a CI, just to make sure?

ernestmc · 2016-02-05T17:57:02Z

😄 I sure will @esteve. Thank you guys!

ernestmc · 2016-02-05T18:47:22Z

Squashed and triggered CI jobs:

ernestmc · 2016-02-05T19:55:59Z

I get a warning in Linux but seems to be in the gtest code: http://ci.ros2.org/job/ci_linux/906/warnings17Result/
Besides that it looks good to merge.

dirk-thomas · 2016-02-05T20:16:15Z

rosidl_generator_cpp/test/test_interfaces.cpp

@@ -34,6 +42,9 @@
 #include "rosidl_generator_cpp/msg/unbounded_array_static.hpp"
 #include "rosidl_generator_cpp/msg/unbounded_array_unbounded.hpp"

+#define TEST_VECTOR_SIZE 10
+#define TEST_VECTOR_SIZE_2 3


The constant name is not very helpful. Is this the vector size for nested messages?

Yes, the first one is the size of the primitives array and the second one is the size of nested message array.

What about calling it TEST_NESTED_VECTOR_SIZE then?

I changed it to something more meaningful.

dirk-thomas · 2016-02-05T20:18:15Z

The warning seems to point to an empty line test_interfaces.cpp:120: https://github.com/ros2/rosidl/pull/64/files#diff-8402e744cd022a18b9569d4f75323991R120 Can you please check what the real line resulting in this warning is? I guess there will be a way to use gtest differently to avoid the warning. Worst case we could always exclude the specific warning as a last resort.

ernestmc · 2016-02-05T20:31:32Z

@dirk-thomas according to this:

This is a GCC bug.  gtest isn't trying to actually covert false to a pointer.  
It just uses a Template Meta Programming trick that's blessed by the C++ 
standard to detect NULL literals.

Maybe we can exclude that warning?

gerkey · 2016-02-09T21:51:30Z

@ernestmc Go ahead and disable that warning, then we'll be ready to merge. Suggest that you try a GCC-specific #pragma to disable the warning as narrowly as possible in the code. Similar to this.

ernestmc · 2016-02-10T17:21:07Z

Locally disabled warnings:

rosidl/rosidl_generator_cpp/test/test_interfaces.cpp

Line 130 in 8561073

#ifdef __linux__

Cpp tests

esteve · 2016-02-10T17:40:16Z

@ernestmc 🎉 🎈 awesome! I've deleted the branch, we delete the branch of a PR after it's been merged, but sometimes we forget too :-)

ernestmc · 2016-02-10T17:46:27Z

👌

ernestmc added the in progress Actively being worked on (Kanban column) label Jan 12, 2016

dirk-thomas reviewed Jan 13, 2016
View reviewed changes

ernestmc force-pushed the cpp_tests branch from 29411f3 to da5a389 Compare January 26, 2016 18:50

jacquelinekay reviewed Jan 27, 2016
View reviewed changes

ernestmc added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 4, 2016

ernestmc force-pushed the cpp_tests branch 2 times, most recently from cf3c862 to 8bb45b5 Compare February 5, 2016 18:42

dirk-thomas reviewed Feb 5, 2016
View reviewed changes

ernestmc force-pushed the cpp_tests branch from 8bb45b5 to 374bb6b Compare February 5, 2016 20:42

ernestmc force-pushed the cpp_tests branch from 374bb6b to 8561073 Compare February 10, 2016 17:16

Added tests for cpp generated message types.

d33a3b7

ernestmc force-pushed the cpp_tests branch from 8561073 to d33a3b7 Compare February 10, 2016 17:32

ernestmc added a commit that referenced this pull request Feb 10, 2016

Merge pull request #64 from ros2/cpp_tests

ead28e7

Cpp tests

ernestmc merged commit ead28e7 into master Feb 10, 2016

ernestmc removed the in review Waiting for review (Kanban column) label Feb 10, 2016

esteve deleted the cpp_tests branch February 10, 2016 17:40

tfoote mentioned this pull request Feb 11, 2016

Add tests to rosidl_generator_cpp ros2/ros2#168

Closed

Cpp tests #64

Cpp tests #64

Conversation

ernestmc commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestmc commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquelinekay commented Jan 28, 2016

ernestmc commented Jan 29, 2016

jacquelinekay commented Feb 3, 2016

jacquelinekay commented Feb 3, 2016

ernestmc commented Feb 3, 2016

ernestmc commented Feb 4, 2016

jacquelinekay commented Feb 4, 2016

jacquelinekay commented Feb 4, 2016

ernestmc commented Feb 4, 2016

jacquelinekay commented Feb 4, 2016

ernestmc commented Feb 4, 2016

jacquelinekay commented Feb 4, 2016

jacquelinekay commented Feb 4, 2016

ernestmc commented Feb 4, 2016

ernestmc commented Feb 5, 2016

ernestmc commented Feb 5, 2016

dirk-thomas commented Feb 5, 2016

ernestmc commented Feb 5, 2016

jacquelinekay commented Feb 5, 2016

esteve commented Feb 5, 2016

dirk-thomas commented Feb 5, 2016 • edited Loading

ernestmc commented Feb 5, 2016

esteve commented Feb 5, 2016

ernestmc commented Feb 5, 2016

ernestmc commented Feb 5, 2016

ernestmc commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Feb 5, 2016

ernestmc commented Feb 5, 2016

gerkey commented Feb 9, 2016

ernestmc commented Feb 10, 2016

esteve commented Feb 10, 2016

ernestmc commented Feb 10, 2016

dirk-thomas commented Feb 5, 2016 •

edited

Loading