D19 Comments and Diff

[Kamil Paral]

> > I also want to avoid increasing the maintenance burden of having a
> > bunch of tests that look for things which don't really need to be
> > tested (setting data members, checking default values etc.)
> 
> I agree, there is stuff that kind of can be taken for granted.

Coincidentally, I have an anecdote to tell... of this morning.

I have went ahead and implemented Tim's suggestion here:
https://phab.qadevel.cloud.fedoraproject.org/D19?id=49#inline-127

A damn simple change, just replace (slightly modified for readability):
self._result = None
with
self._result = 'NEEDS_INSPECTION'
in the constructor and a trivial adjustment in the result getter.

However, my 'trivial' tests actually saved me:

=================================== FAILURES ===================================
_______________________ TestCheckDetail.test_init_raise ________________________

self = <testing.test_check.TestCheckDetail instance at 0x2fc1d88>

    def test_init_raise(self):
        '''Test instance creation that raises an error - invalid parameters'''
        with pytest.raises(exc.TaskotronValueError):
>           check.CheckDetail(result='INVALID TYPE')
E           Failed: DID NOT RAISE

testing/test_check.py:38: Failed
______________________ TestCheckDetail.test_update_result ______________________

self = <testing.test_check.TestCheckDetail instance at 0x343f638>

    def test_update_result(self):
        '''Test 'update_result' method'''
        # basic use case
        self.cd.update_result('FAILED')
>       assert self.cd.result == 'FAILED'
E       assert 'NEEDS_INSPECTION' == 'FAILED'
E         - NEEDS_INSPECTION
E         + FAILED

testing/test_check.py:66: AssertionError
================ 2 failed, 50 passed, 1 skipped in 0.32 seconds ================

So, even those simple tests have some value, sometimes.

Of course, there's no point in testing whether simple "class.attr = value" statement really works. But if attr is a property (having setter/getter), now it starts to make sense. And also if the documentation says it should have some default value (other than None), it's useful to test it. I don't think it's strictly required, but I definitely wouldn't call it unnecessary.