Improve SDK test coverage

[jcapona]

Status	Ticket/Issue
Ready/Hold	Ticket

Main changes

-

Screenshots (feature, test output, profiling, dev tools etc)

[insert screenshots here]

Other notes (e.g. implementation quirks, edge cases, questions / issues)

-

Manual testing tips

-

Tag anyone who definitely needs to review or help

-

[codecov[bot]]

Codecov Report

Merging #522 (e33cbae) into master (45fa20f) will increase coverage by 19.55%. The diff coverage is 66.98%.

@@             Coverage Diff             @@
##           master     #522       +/-   ##
===========================================
+ Coverage   29.52%   49.07%   +19.55%     
===========================================
  Files         171      146       -25     
  Lines        9196     6877     -2319     
===========================================
+ Hits         2715     3375      +660     
+ Misses       6481     3502     -2979     

Flag	Coverage Δ
unittests	49.07% <66.98%> (+19.55%)	:arrow_up:

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

Impacted Files	Coverage Δ
pitop/camera/core/cameras/usb_camera.py	19.67% <0.00%> (+0.31%)	:arrow_up:
pitop/miniscreen/oled/core/lock.py	0.00% <0.00%> (ø)
pitop/camera/camera.py	60.71% <37.03%> (+25.89%)	:arrow_up:
pitop/pma/servo_motor.py	67.67% <44.44%> (+23.23%)	:arrow_up:
pitop/common/sys_info.py	84.78% <82.35%> (+84.78%)	:arrow_up:
pitop/common/ptdm.py	91.56% <97.22%> (+27.38%)	:arrow_up:
pitop/camera/core/frame_handler.py	90.56% <100.00%> (+56.60%)	:arrow_up:
pitop/keyboard/vendor/pynput/keyboard/_win32.py
pitop/keyboard/vendor/pynput/mouse/_darwin.py
pitop/keyboard/vendor/pynput/keyboard/_xorg.py
... and 58 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 45fa20f...e33cbae. Read the comment docs.

[olivierwilkinson]

When running on my mac (after making a new venv and installing deps from /tests/requirements.txt) I get the following errors. It looks like the face and emotion detector tests need the dlib python library and the sys info tests need certain directories to exist. I think we should add dlib to the requirements.txt file or mock it out. The sys info tests shouldn't be checking real directories so I think they need some better isolation.

[olivierwilkinson]

So the dlib stuff does install from the requirements.txt fine but my installation of it's deps are messed up from using brew for things so I think that's just my problem. I do still get one sys info test failing consistently though:

If you want to jump on a call so we can debug it on my machine I'm very happy to do that whenever :)

[angusjfw]

the helper being tested is checking for the existence of '/etc/debian_version' so clearly doesn't work outside debian... it's literally called get_debian_version.

this could be fixed with path_mock, as in the following test, but IMO that leaves us will a less useful test.

all the helper does is read this specific file, it could only really break if the file were to be removed from debian. I think it might be better to skip the test when not on debian, rather than mock it's assumptions entirely.

[olivierwilkinson]

the helper being tested is checking for the existence of '/etc/debian_version' so clearly doesn't work outside debian... it's literally called get_debian_version.

this could be fixed with path_mock, as in the following test, but IMO that leaves us will a less useful test.

all the helper does is read this specific file, it could only really break if the file were to be removed from debian. I think it might be better to skip the test when not on debian, rather than mock it's assumptions entirely.

Testing whether the file actually exists on the system that is running the test goes beyond what a unit test is. These tests are definitely under the unit test criteria, regardless of whether it's debatable what falls under that banner there is no structure around what the environment they are run in so they can't be considered integration or e2e tests. Even if we did set up locally to run the tests on debian or ubuntu that isn't even our actual target environment...

I think we should ensure this test behaves as a unit test by mocking the actual check and then properly address the issue of integration / e2e testing separately

[jcapona]

@olivierwilkinson I've added a few missing path.exists() mocks so it should be good now 🤞🏼

[olivierwilkinson]

@olivierwilkinson I've added a few missing path.exists() mocks so it should be good now 🤞🏼

Works for me now! 🎉🙌