CaringCaribou / caringcaribou

A friendly car security exploration tool for the CAN bus
GNU General Public License v3.0
751 stars 197 forks source link

Adding unittests for listener module #127

Closed n135c10r closed 2 months ago

n135c10r commented 2 months ago

Adding pytest as a dev dependency.

Im not very proficient in testing, but I plan to improve my skills by adding tests for rest of the modules 🎃

I also want to change some of the existing tests, I think we can improve the mocking there, because as i can see its requiring a real interface to be connected during the test, so i think now it looks more like a system tests.

BTW i tried to do those patches as fixtures and move it to conftest.py but unfortunately it was not working 🫤 Its quite tricky to handle the KeyboardInterrupt during the test.

kasperkarlsson commented 2 months ago

Hello again! First off, thanks for sending another PR!

My thoughts on (and plans/wishes for) testing in Caring Caribou are somewhere along these lines:

As for this PR, the vision you describe sure sounds promising but I do not see much value being added yet. We get an extra dependency but not much more relevant testing, and I do not wish for you to feel overwhelmed by an only-just-begun feature which has already been put into the master branch. If the goal is to replace/extend large parts of the testing, this might be better to do in a separate branch and merged into master once it adds substantial value.

Please let me know what you think, I would be more than happy to hear your perspective here as well! 😃

n135c10r commented 2 months ago

Ahh i understand, you are totaly right, this is a very mature approach 👍 I will create more tests on my fork, and then submit a pull request.

kasperkarlsson commented 2 months ago

All right! You may want to consider doing so in a separate branch of your fork to keep it isolated, in case you wish to be able to develop other features/fixes before the revamped testing is done 🙂👍