[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Regarding test for the other checks, I'll try to fabricate a few invalid minidumps (although it would obviously provide limited coverage)



================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
     ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    if (expect_failure)
----------------
amccarth wrote:
> lemo wrote:
> > amccarth wrote:
> > > I would rather see this function return the result of the Initialize and let the individual tests check the expectation explicitly.
> > > 
> > > I know that will lead to a little bit of duplication in the tests, but it will make the individual tests easier to understand in isolation.  It also makes it possible for each test to decide whether to ASSERT or EXPECT.  And it eliminates the need for the bool parameter which is hard to decipher at the call sites.
> > Good idea, although gunit doesn't let us ASSERT in non-void returning functions.
> > 
> > I agree that the bool parameter is ugly, so I created a separate TruncateMinidump() helper (which cleans up the SetUpData since the load_size was only used for truncation)
> This isn't quit what I meant.  I'd rather not have the ASSERTs in helper functions at all (regardless of return type).  The helpers should return a status and the actual test code should do whatever ASSERT or EXPECT is appropriate.
So how would we handle the existing checks in SetupData()?


https://reviews.llvm.org/D49202