pointer check in unit tests

[Mike Kaganski]

On 03.11.2020 11:37, Miklos Vajna wrote:
> Hi Regina,
> 
> On Mon, Nov 02, 2020 at 07:52:54PM +0100, Regina Henschel <rb.henschel at t-online.de> wrote:
>> ScDocShellRef xDocSh = loadDoc("tdf137020_FlipVertical.", FORMAT_ODS);
>> CPPUNIT_ASSERT_MESSAGE("Failed to load tdf137020_FlipVertical.ods",
>> xDocSh.is());
>> ScDocument& rDoc = xDocSh->GetDocument();
>> ScDrawLayer* pDrawLayer = rDoc.GetDrawLayer();
>> CPPUNIT_ASSERT_MESSAGE("No SdDrawLayer", pDrawLayer);
>> SdrPage* pPage = pDrawLayer->GetPage(0);
>> CPPUNIT_ASSERT_MESSAGE("No draw page", pPage);
>> CPPUNIT_ASSERT_EQUAL( static_cast<size_t>(1), pPage->GetObjCount() );
>> SdrObject* pObj =  pPage->GetObj(0);
>> CPPUNIT_ASSERT_MESSAGE("No object", pObj);
>>
>> Can I drop the CPPUNIT_ASSERT in these cases? They have nothing to do with
>> the test itself, but check only the pointers, which appear on the way to the
>> object, which I want to test.
> 
> The benefit of such explicit asserts is that they document your
> assumptions and if they are not held, then a test failure log will show
> this failure explicitly.
> 
> If you don't do that, we'll only see that the test crashed.
> 
> I believe that in most cases this benefit is larger than the cost of the
> noise in the code.
> 
> (If you see a case where a pointer is returned and it can't be ever
> nullptr, then we should fix the return type to a be reference. Caolan
> did lots of fixes like that recently.)

Note also that all calls to ScBootstrapFixture::loadDoc currently either 
assert on its result, or dereference it unconditionally; so this 
function may be improved to include the assert inside, and guarantee the 
validity of returned result. One assert less in unit test code :-)

-- 
Best regards,
Mike Kaganski