test, dev: fix mocking in test suite, enable dev to run single tests, get coverage report

[redshiftzero]

Backstory

When I went to do some bug fixes on Friday, I first ran the test suite on master and found that the test suite was failing locally for me - this PR description contains the main problems I hit and how they are resolved here.

Problem 1: defining but not applying our inline unittest.mock.patch mocks

The first error I hit was due to lsblk not being present on my system (i was on a mac while doing this which doesn’t have lsblk). Investigating further, I was surprised to see that the test failed since the subprocess call to lsblk was mocked.

That’s when I realized the problem: in places we were using unittest.mock.patch inline but not calling .start() on the mocks. If we don’t do this, they won’t be applied: see this branch which has the mocks started which causes many test failures.

Fix: we can use pytest-mock for our inline mocks: when we use this we just need to use the mocker pytest fixture and the mock will be started and stopped for us.

Problem 2: mocking in the wrong place

In that branch’s test output we see the second issue - we have export.exit_gracefully mocked - but exit_gracefully is a method on SDExport, not a module-level function (this is seen via the test output above when we try to start the mock) . When submission is an instance of SDExport we should mock the exit_gracefully method on the submission object.

Considering the logic in the asserts here, it doesn’t make sense: if exit_gracefully is properly mocked (where sys.exit(0) is occurring most of the time) then we should not need to assert that a SystemExit is raised unless we include sys.exit(0) as a side effect of the mocked exit_gracefully. This lets us know that the mock isn't working.

It’s OK to mock out the exit_gracefully method calls since there are unit tests for that method which cover both the exception / no exception cases.

If you're wondering: but... we had asserts on mocked_exit - why did the asserts pass? It turns out they didn't - this test run based on a one line change here demonstrates this. This was masked due to the fact they were in the pytest.raises context and were not executed. See this comment in the pytest source code for what is going on. The remaining uses of pytest.raises in this PR do not have additional asserts so we should be good now on this front but it's something to be aware of.

Fix: we ensure that we’re mocking exit_gracefully in the right place and removing the unnecessary asserts (that will not pass due to no longer running the logic inside exit_gracefully)

other dev/test improvements

I added some other minor dev agility and test improvements in the course of working on this (let me know if you want me to break this up for easier review, can do so):

dev:

• enable a dev to run single tests
• add coverage output

test:

• Ensure that we use side_effect (instead of return_value) so that exceptions are raised
• I’ve broken up the longer tests with whitespace to delineate the build, operate, check (AKA arrange, act, assert) sections of the test to make them a bit easier to follow

anyway please let me know if you think I misunderstood anything here!

[emkll]

Thanks @redshiftzero for the comprehensive description of the issues and the significant improvements to the test suite. Changes look good to me, I'll let @creviera do a final review/merge since they marked this as under review.

[sssoleileraaa]

changes lgtm