test: :white_check_mark: add activation_and_receive_command

[jaron-l]

• also change updateController to use sleeps instead of CPU intense while loop

refs: #421

[codecov-commenter]

Codecov Report

Merging #441 (4bcf772) into master (e7f9962) will decrease coverage by 5.79%. The diff coverage is 20.65%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   35.78%   29.98%   -5.80%     
==========================================
  Files         189        7     -182     
  Lines       17570      737   -16833     
  Branches    11592      422   -11170     
==========================================
- Hits         6287      221    -6066     
+ Misses        994      161     -833     
+ Partials    10289      355    -9934     

Flag	Coverage Δ
unittests	29.98% <20.65%> (-5.80%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp	100.00% <ø> (ø)
...ontroller/test/test_load_diff_drive_controller.cpp	12.50% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp	42.16% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp	17.62% <12.08%> (ø)
diff_drive_controller/src/speed_limiter.cpp	46.55% <13.33%> (ø)
...troller/include/diff_drive_controller/odometry.hpp	20.00% <20.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp	32.67% <24.89%> (ø)
...ler/test/test_load_joint_trajectory_controller.cpp
...test/test_load_joint_group_position_controller.cpp
...test/test_load_joint_group_position_controller.cpp
... and 192 more

[progtologist]

I am not sure about the activation of the unit test that was commented out, but I can comment on this:

also change updateController to use sleeps instead of CPU intense while loop The code that replaced it does not do the same thing. The test is supposed to run update as fast as possible for wait_time duration, while the code that replaces it only runs update twice. I don't think the test was intended to do that.

[jaron-l]

I am not sure about the activation of the unit test that was commented out, but I can comment on this:

also change updateController to use sleeps instead of CPU intense while loop The code that replaced it does not do the same thing. The test is supposed to run update as fast as possible for wait_time duration, while the code that replaces it only runs update twice. I don't think the test was intended to do that.

Are you suggesting that I should separate the PR? The reason I included it was because it was related to the failure mode that the test would have if simply commented in. Though, yes, I could technically separate the fixes, I thought it trivial to add in as the PR is small.

If you're suggesting that the change to updateController was unwarranted, then I'm interested in hearing why. My experience with the function is that it unnecessarily made tests take longer and would make my CPU ramp up unnecessarily because of the run-as-fast-as-possible loop, which I think is generally avoided as it will restrict the ability of other threads to run. Also, I don't get why running the update function would be needed more than twice anyways. Perhaps you could explain that?

[progtologist]

Are you suggesting that I should separate the PR? The reason I included it was because it was related to the failure mode that the test would have if simply commented in. Though, yes, I could technically separate the fixes, I thought it trivial to add in as the PR is small.

No, I agree with you that the change is quite small so I think that it could be included in this PR.

If you're suggesting that the change to updateController was unwarranted, then I'm interested in hearing why. My experience with the function is that it unnecessarily made tests take longer and would make my CPU ramp up unnecessarily because of the run-as-fast-as-possible loop, which I think is generally avoided as it will restrict the ability of other threads to run. Also, I don't get why running the update function would be needed more than twice anyways. Perhaps you could explain that?

I am not sure how the change you suggested would be shorter, as it should still take the same amount of time, the only difference being that on the one unit test it runs in full tilt for wait_time, while in your suggestion it sleeps for wait_time. It should also not restrict the ability of other threads to run, as it runs on a single thread, all the other threads of your system should be available to perform other tasks on your computer.

Now regarding why running the update function more than twice is needed, I find it hard to believe that the original author who wrote the unit test wrote a while loop like that instead of a simple call_function(); sleep(); call_function(); without good reason.

My comment in your code is just stating the obvious, that the code you replaced is not 1-1 equivalent. It doesn't run the exact same thing but in a more efficient way, it modifies what the function does.

Lastly, this function is called a few lines below, so it seems required to call update a few times for these checks to pass, is it not?

[jaron-l]

I am not sure how the change you suggested would be shorter, as it should still take the same amount of time, the only difference being that on the one unit test it runs in full tilt for wait_time, while in your suggestion it sleeps for wait_time. It should also not restrict the ability of other threads to run, as it runs on a single thread, all the other threads of your system should be available to perform other tasks on your computer.

Now regarding why running the update function more than twice is needed, I find it hard to believe that the original author who wrote the unit test wrote a while loop like that instead of a simple call_function(); sleep(); call_function(); without good reason.

My comment in your code is just stating the obvious, that the code you replaced is not 1-1 equivalent. It doesn't run the exact same thing but in a more efficient way, it modifies what the function does.

I see your point. The change in the function is not all that different is that it just replaces a while-loop running for a period of time with some sleeps for roughly that same period of time. But I would argue that this methodology is not good practice for a unit test. I would think we would want our tests to run as fast as possible and not depend on a certain period of time passing. I tried implementing this without the sleeps but one of the tests crashes when accessing uninitialized memory and would require some bug fixing. I would guess that the while loop was originally added for the same reason I added it, because they couldn't get it to work otherwise and don't quite get the complexity of the tests. Perhaps I should remove the change to updateController that I've made in this PR and set it aside for another PR that improves the performance of updateController in addition to fixing the tests that don't use the same clock source as updateController (which is why I think they depend on a certain amount of time passing). What do you think if that idea?

Lastly, this function is called a few lines below, so it seems required to call update a few times for these checks to pass, is it not?

The tests run just fine with two calls to update. I believe this is because I'm essentially setting up a step function in updateController that has a period set before and after the trajectory message is active and so the update function calculates the control to make the step function achieve the desired period.

[jaron-l]

On further inspection, I'm not sure I see the value of adding this test back in. As I see it, the test mainly has two components:

1. It tests that the controller gets activated appropriately.
2. It tests that a command is received and that the controller position goes to that position

These two parts are already tested in two different unit tests. The first one is tested in TrajectoryControllerTestParameterized.activate and the second one is tested in TrajectoryControllerTestParameterized.correct_initialization_using_parameters. What if I just deleted this test?

[mergify[bot]]

This pull request is in conflict. Could you fix it @jaron-l?