UCSolarCarTeam / Epsilon-Embedded-Software

4 stars 1 forks source link

SFT-169 Write Driver Controls Unit Tests #174

Closed JessieG123 closed 3 years ago

JessieG123 commented 3 years ago

Created DriverControls unit test following https://docs.google.com/document/d/1eOP1Gx6FoK7ut762bZeflf8Is09Y0-ksVAQKfRJpOag/edit?usp=sharing.

Note: If HAL_ADC_PollForConversion_ExpectAndReturn function in regenPercentageGreaterThanNonZeroThresholdAndAllowChargeIsFalse is commented out, it will affect reverseIsTrueAndAccelpercentageGreaterThanNonZeroThreshold and forwardIsTrueAndAccelpercentageGreaterThanNonZeroThreshold which is most likely a very weird Unity error. With the reverse one, the test is setting reverse as true but when the values are printed out, it always says forward is true when that HAL_ADC function is commented out.
Temporarily resolved by putting the reverse and forward test above the regen one.

badeniran commented 3 years ago

First Pass: Verifying the coverage - doing the following in DriverControl.c does not break the tests, but probably should (can be fixed here or a different PR):

  • Commenting out line 232/239 (The conversions of calues for newRegen/newAccel)
  • Changing the allowCharge mask from 0x02->0x03
  • Commenting out line 264 - 265 (checking how safe the new direction is)
  • Commenting out lines 288 - 289 (mechanical brake functionality)

As for that weird unity issue, I think it's because:

  • The regenPercentageGreaterThanNonZeroThresholdAndAllowChargeIsFalse test has an expect of HAL_GPIO_ReadPin_ExpectAndReturn(FORWARD_GPIO_Port, FORWARD_Pin, 0);.
  • Because the test failed and ended early before sendDriveCommands could call HAL_GPIO_ReadPin, the expect is still in cmock's "queue".
  • On the next test in regenPercentageGreaterThanNonZeroThresholdAndAllowChargeIsFalse, there is a call to HAL_GPIO_ReadPin_ExpectAndReturn(FORWARD_GPIO_Port, FORWARD_Pin, 1);
  • However when sendDriveCommands is called and gets to that line, it'll look at the first one in queue, where it is set to forward = 0, and runs from there, failing the test.

If that clears things up, I think that's intended behavior of CMock, so some of the clarifying comments can be dropped.

Yeah your thinking sounds correct. I looked around and found that there's a resetTest function that's apparently supposed to be called and do clean up of the CMock queue, but I had a hard time finding where it's located and how to call it. That would probably fix all weird issues down the line, so it may be something to look into.

bill-luu commented 3 years ago

Oh dang if we can figure out the resetTest function that make life a lot easier! For now a nice LPT to pass along is when adding a test, try placing it as the very first test, and if that works then move it to the very last test. Hopefully that should catch any ordering issues!

JessieG123 commented 3 years ago

Oh dang if we can figure out the resetTest function that make life a lot easier! For now a nice LPT to pass along is when adding a test, try placing it as the very first test, and if that works then move it to the very last test. Hopefully that should catch any ordering issues!

From it's documentation, it seems like when you use Unity's generate_test_runner.rb helper script it creates the resetTest function that can be called. The generate_test_runner.rb file is in the auto folder in Unity which we don't have.

https://github.com/ThrowTheSwitch/Unity/blob/master/auto/generate_test_runner.rb https://github.com/ThrowTheSwitch/Unity/blob/master/docs/UnityHelperScriptsGuide.md

badeniran commented 3 years ago

Oh dang if we can figure out the resetTest function that make life a lot easier! For now a nice LPT to pass along is when adding a test, try placing it as the very first test, and if that works then move it to the very last test. Hopefully that should catch any ordering issues!

From it's documentation, it seems like when you use Unity's generate_test_runner.rb helper script it creates the resetTest function that can be called. The generate_test_runner.rb file is in the auto folder in Unity which we don't have.

https://github.com/ThrowTheSwitch/Unity/blob/master/auto/generate_test_runner.rb https://github.com/ThrowTheSwitch/Unity/blob/master/docs/UnityHelperScriptsGuide.md

It's here /UnityTestingTool/CMock/vendor/unity/auto/generate_test_runner.rb

JessieG123 commented 3 years ago

Oh dang if we can figure out the resetTest function that make life a lot easier! For now a nice LPT to pass along is when adding a test, try placing it as the very first test, and if that works then move it to the very last test. Hopefully that should catch any ordering issues!

From it's documentation, it seems like when you use Unity's generate_test_runner.rb helper script it creates the resetTest function that can be called. The generate_test_runner.rb file is in the auto folder in Unity which we don't have. https://github.com/ThrowTheSwitch/Unity/blob/master/auto/generate_test_runner.rb https://github.com/ThrowTheSwitch/Unity/blob/master/docs/UnityHelperScriptsGuide.md

It's here /UnityTestingTool/CMock/vendor/unity/auto/generate_test_runner.rb

Oh sounds good. I'll try to see if I can do something with it.

badeniran commented 3 years ago

I think it's good to merge. Might want to make looking into the test generation script a separate task

Actually, make sure it's up to date with master first, then squash and merge