Add CI Testing Section #87

gramss · 2020-09-21T22:42:23Z

Hey,

I just explored CI testing for myself and had the feeling of writing about what the spirit of CI testing is and what entry points for CI beginners might be interesting.

Also included is my example of a working partial script of RewrittenYaml.

Hope this fits into the documentation?

Cheers

SteveMacenski

See second comment: I think this needs a better understanding of audience and goal. I'm not sure what the reader walks away from this with or who its targeted at. It doesn't really explain how to write any given type of test or the tools around it. It also isn't detailed enough to act as a desk reference for codecov / gtesting / rules.

SteveMacenski · 2020-09-22T17:42:11Z

tutorials/docs/ci_testing.rst

+Benefits of Automated Testing
+=============================
+
+.. image:: images/CI_Testing/code_coverage_bump_40_80.png


What's the point this image is trying to convey?

Basically "stonks" (the meme).

A growing coverage should motivate others to help raise the bar to 100 (or as you more realistically said: 90%).
And they should question themselves what it takes to create such a huge bump from 40 to ~75.
Also, they should see that it can be big jumps and also small jumps (new tests added) that also help improve.

So basically multiple things, that also motivated me to write something about it..

Disclaimer: I'm absolutely new to CI / testing. I just wrote down my understanding of things while I discovered them. So thanks for the input! I'll try to improve the points you mentioned and learn a few things more on the go.

Lets not use this image, its deceptive, its a little more complex to describe why that jump happened, but its not as obvious as you think

SteveMacenski · 2020-09-22T17:42:33Z

tutorials/docs/ci_testing.rst

+Whenever a feature gets added or altered, it is best practice to confirm the intended outcome with automated test scripts.
+This document features an easy step by step guide how CI should be integrated alongside new PRs.
+
+Also, not only new code should have CI testing. To find nasty bugs and corner cases, it is Nav2's ultimate goal to have a test coverage of nearly 100% !


.... don't make promises for me ;-)

Lets change that to 90%. That's my goal for every individual package.

sure thing.. :-) done.

SteveMacenski · 2020-09-22T17:44:59Z

tutorials/docs/ci_testing.rst

@@ -0,0 +1,224 @@
+.. _ci_testing:


So the big things here are:

Audience: who is this document for?

Goals: What is this document trying to accomplish?

Its not clear to me what this is trying to do. I don't disagree having some content about testing in documentation, but I'm not sure what the aim or audience this document has.

"CI" seems odd here too. You mostly talk about testing, almost nothing about CI beyond the code coverage report.

Audience:
Get new folks involved with testing. So programmers that already added some code and want to nicely finish their PR with tests but have no clue where to even start looking in a massive project like nav2.
Testing does not take ages to figure it out by themself, but it should be just something that gives you some good starting points and get to the point and continue programming

Goal:
Make clear to them that testing is not hard and can help you to get to know your code better

CI and testing where synonyms for me at the beginning of this document. Testing in CI is also just a little chapter of CI.
Probably should remove CI references in the file names..
But changed the setting now a bit. Let me hear what you think.

So why is it called "CI testing" if its meant for new people learning about testing?

I'm not sure this really hits the target on helping people get into testing. There's not really a linear narrative explaining concepts, showing how to use tools, and helping write tests. There's no real mention or details on gtest and test-launch and the ament testing tools, which I'd think would be necessary for a "how do I setup tests" tutorial. There's not any code to be a "writing my first test" tutorial. There's not really a clear direction this content is going in - seems to me like a bunch of assembled facts but isn't guiding a reader towards understanding any particular aspect of testing.

SteveMacenski · 2020-09-22T17:46:03Z

tutorials/docs/ci_testing.rst

+1. Run Existing Tests
+---------------------
+Nav2 specific tests can be found in each individual component under a dedicated test folder. 
+One additional package, ``nav2_system_tests``, `exists <https://github.com/ros-planning/navigation2/tree/main/nav2_system_tests>`_ for testing components, sub-systems and full system tests.


", in addition to unit and integration tests in individual packages."

SteveMacenski · 2020-09-22T17:46:49Z

tutorials/docs/ci_testing.rst

+To run existing tests of components or the whole system from within the ``nav2_system_tests`` package, you can use this procedure:
+
+  - build nav2 with ``$ colcon build --symlink-install``
+  - navigate to the component you would like to test ``$ cd build/<package_name>``


This isn't good. You can just do colcon test --packages-select <pkg-name> to run the tests of an individual package. You should never need to go into build.

yes you can do that.
But it is a really really long command this way..:

colcon test --event-handlers console_direct+ --packages-select <pkg-name> --ctest-args -R <regex>

I especially want to see the output via the event-handlers as I need to see what is happening with my new test. The new test mostly is not right the first time. (during normal test times, this should show nothing as it is currently)

Probably still better to use colcon than go to the build folder.

Done.

You're talking about in this section about running "existing tests of components", so you should target that audience. Not the "run a single new test" audience, these are different things.

SteveMacenski · 2020-09-22T17:50:05Z

tutorials/docs/ci_testing.rst

+
+.. code-block:: text
+
+  ament_add_gtest(test_action_spin_action test_spin_action.cpp)


Please explain these cmake macros and what each do

SteveMacenski · 2020-09-22T17:51:08Z

tutorials/docs/ci_testing.rst

+
+.. code-block:: text
+
+  ament_add_test(test_bt_navigator_with_groot_monitoring


This isn't just for python, this is for running any launch-based test vs a raw gtest test. So all these examples are used to run launch files which have test nodes that are launched as well as python. But they dont need to be python.

Changed it.
I thought that launch scripts in ros2 must always be python

SteveMacenski · 2020-09-22T17:52:16Z

tutorials/docs/ci_testing.rst

+Is there room to improve the test coverage near your code? Ain't you just the right expert about this code section?
+Think about adding tests that exceed your own focus and help improve nav2/ros2 reach a higher overall code coverage and ultimately also quality.
+
+The report above is an automated post by codecov.io-bot on github that posts results of CI automatically for every new PR.


I'd also recommend seeing the report and look at the files you modified to see the specific lines that were / weren't covered (or see the new package total %). That can help you see where you need to add more tests for or if you hit your test coverage goal (sometimes there's noise in just that high level % from other packages moving around a little).

SteveMacenski · 2020-09-22T17:53:08Z

tutorials/docs/ci_testing.rst

+
+Rewriting Parameter Values from YAML Files in Launch scripts
+============================================================
+In most occasions some small new features are added and made available through a few new parameters. As the standard nav2 user should not be overloaded with features it makes sense to disable most of the additional or drop-in features in the default ``params.yaml`` file.


The explanation here doesn't 100% explain why this is in this document.

SteveMacenski · 2020-09-22T17:53:41Z

tutorials/docs/ci_testing.rst

+.. note::
+  When testing with ``pytest`` - typically for ros2 launch files -  and building your package with ``$ colcon build --symlink-install``, you can even change your test scripts without rebuilding the whole package! 
+
+3. Writing Your Own Test


Explaining gtest would be good. Or links to launch test uses

also included ref to two similar tutorials by autoware that I came aware of shortly after finishing after what I changed now.. Maybe this covers a few more points that would be interesting as well..?

gramss · 2020-09-23T23:23:00Z

there are 4 uncommented requests from you that I will come back shortly.

… tests

SteveMacenski

I think you pushed your groot stuff to your CI testing PR branch, then deleted your CI testing work.

SteveMacenski · 2020-10-14T19:36:57Z

configuration/packages/configuring-bt-navigator.rst

+  ====== ======= ======= ========
+  Type   Default Unit    Optional
+  ------ ------- ------- --------
+  int    1666    Port    yes


Remove these optional fields, any parameter with a default is technically optional. As well with unit, please stick with the format unless you'd like to update all of the parameters in all of the guides to this new format 😉

SteveMacenski · 2020-10-14T19:37:35Z

tutorials/docs/groot.rst

@@ -0,0 +1,88 @@
+.. _groot_introduction:


Groot tutorial and deleted the CI testing tutorial? I'm confused, this is the CI testing section PR.

SteveMacenski · 2020-10-15T17:57:38Z

Please revert all groot stuff from this PR and open a new PR with groot docs. I won't review this PR with 2 different things in it that aren't related.

gramss added 3 commits September 20, 2020 22:43

small fix in rst highlighting

26b334f

fix the readme and build instructions to just use CI settings

aad3d13

add CI Testing section to nav2-documentation incl. rewritting yaml parms

17dd8e8

SteveMacenski requested changes Sep 22, 2020

View reviewed changes

tried to improve all the points mentioned above

ef3e83f

also included ref to two similar tutorials by autoware that I came aware of shortly after finishing after what I changed now.. Maybe this covers a few more points that would be interesting as well..?

gramss added 7 commits September 27, 2020 06:56

add groot monitoring parameters to documentation

e6b3615

add linter section

1f3081a

adding section about writing pytest test

97fe1c9

misc - adding a few lines for clarification

7a22c28

renaming files

01782d1

removing code_coverage_bump

7ee19cc

add groot intro for BT editing and monitoring

9126c1b

SteveMacenski mentioned this pull request Sep 28, 2020

Tutorial: local test and coverage generation #88

Open

gramss added 4 commits October 12, 2020 02:48

rename file

de7d27c

add cleaned/generalized version of ros-navigation#88 - local code-cov…

5b91f7a

… tests

Update Groot to include the merged PR

985cb1f

gtest + code coverage from ros-navigation#88

1f028de

SteveMacenski requested changes Oct 14, 2020

View reviewed changes

add Groot to foxy migration guide

fd7fd38

gramss mentioned this pull request Nov 12, 2020

Groot Documentation #113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CI Testing Section #87

Add CI Testing Section #87

gramss commented Sep 21, 2020

SteveMacenski left a comment •

edited

SteveMacenski Sep 22, 2020

gramss Sep 23, 2020

SteveMacenski Sep 24, 2020

SteveMacenski Sep 22, 2020

gramss Sep 23, 2020

SteveMacenski Sep 22, 2020

gramss Sep 23, 2020

SteveMacenski Sep 24, 2020 •

edited

SteveMacenski Sep 22, 2020

gramss Sep 23, 2020

SteveMacenski Sep 22, 2020

gramss Sep 23, 2020

SteveMacenski Sep 24, 2020

SteveMacenski Sep 22, 2020

SteveMacenski Sep 22, 2020

gramss Sep 23, 2020

SteveMacenski Sep 22, 2020

SteveMacenski Sep 22, 2020

SteveMacenski Sep 22, 2020

gramss commented Sep 23, 2020

SteveMacenski left a comment

SteveMacenski Oct 14, 2020

SteveMacenski Oct 14, 2020

SteveMacenski commented Oct 15, 2020 •

edited


		.. code-block:: text

		ament_add_gtest(test_action_spin_action test_spin_action.cpp)


		.. code-block:: text

		ament_add_test(test_bt_navigator_with_groot_monitoring

Add CI Testing Section #87

Are you sure you want to change the base?

Add CI Testing Section #87

Conversation

gramss commented Sep 21, 2020

SteveMacenski left a comment • edited

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

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

SteveMacenski Sep 24, 2020 • edited

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

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

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

gramss commented Sep 23, 2020

SteveMacenski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Oct 15, 2020 • edited

SteveMacenski left a comment •

edited

SteveMacenski Sep 24, 2020 •

edited

SteveMacenski commented Oct 15, 2020 •

edited