lemo added a comment.
Thanks Adrian for the thorough review.
================
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:
> > > 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()?
> SetUpData would just return an error status instead of ASSERTing. The actual
> ASSERTs would be placed in the tests that call SetUpData. As I said, that
> would add some duplication, because individual tests would have to ASSERT (or
> EXPECT) on the result of SetUpData, but it makes the tests easier to read and
> maintain the tests.
>
> Since there are other tests using SetUpData, they would have to be updated,
> so maybe you want to declare this suggestion as out-of-scope for this patch.
> Anyway, I'm happy that you eliminated the boolean argument.
I think I understand the idea and I agree it's tempting. The problem is that
not all the checks in SetupData() are status based, so there's no direct
mapping from each operation to a return value.
It doesn't seem terribly bad to let the helper encapsulate some of the checks.
https://reviews.llvm.org/D49202
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
