Refactor common code for ensuring core dump for unit tests.

Refactor common code for ensuring core dump for unit tests.

This CL moves the common code for ensuring a core dump file being
generated for a unit test to CrashGenerator::CreateCoreDumpForTest.

[Mark Mentovai]

https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_core_dumper_unittest.cc (right):

https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_core_dumper_unittest.cc:69: pid_t
child_pid = 0;
Unused, no need to initialize. Or just pass NULL.

https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_core_dumper_unittest.cc:75: return;
The condition takes up multiple lines, and it’s hard to follow the scoping
because of the ragged indentation, so use {} around this.

https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
File src/common/linux/tests/crash_generator.cc (right):

https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
src/common/linux/tests/crash_generator.cc:265: fprintf(stderr, "%s test is
skipped due to failure to crash a process\n",
Maybe the other two “return false” cases shouldn’t, but don’t you think that
this case should still cause the test to fail?

[Ben Chan]

https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
File src/client/linux/minidump_writer/linux_core_dumper_unittest.cc (right):

https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_core_dumper_unittest.cc:69: pid_t
child_pid = 0;
On 2015/02/02 17:12:32, Mark Mentovai wrote:
> Unused, no need to initialize. Or just pass NULL.

it's passed to LinuxCoreDumper constructor below on line 92.

https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
src/client/linux/minidump_writer/linux_core_dumper_unittest.cc:75: return;
On 2015/02/02 17:12:32, Mark Mentovai wrote:
> The condition takes up multiple lines, and it’s hard to follow the scoping
> because of the ragged indentation, so use {} around this.

Done.

https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
File src/common/linux/tests/crash_generator.cc (right):

https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
src/common/linux/tests/crash_generator.cc:265: fprintf(stderr, "%s test is
skipped due to failure to crash a process\n",
On 2015/02/02 17:12:32, Mark Mentovai wrote:
> Maybe the other two “return false” cases shouldn’t, but don’t you think that
> this case should still cause the test to fail?

I'm asking myself the same question as well. CreateChildCrash assumes it can
setrlimit and have access to /proc, etc, which may also not be true on certain
platforms. For example, a truncated core file may cause the tests to fail but
it's not really breakpad's fault.

[Ben Chan]

On 2015/02/02 17:52:03, Ben Chan wrote:
>
https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
> File src/client/linux/minidump_writer/linux_core_dumper_unittest.cc (right):
> 
>
https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
> src/client/linux/minidump_writer/linux_core_dumper_unittest.cc:69: pid_t
> child_pid = 0;
> On 2015/02/02 17:12:32, Mark Mentovai wrote:
> > Unused, no need to initialize. Or just pass NULL.
> 
> it's passed to LinuxCoreDumper constructor below on line 92.
> 
>
https://breakpad.appspot.com/4714002/diff/1/src/client/linux/minidump_writer/...
> src/client/linux/minidump_writer/linux_core_dumper_unittest.cc:75: return;
> On 2015/02/02 17:12:32, Mark Mentovai wrote:
> > The condition takes up multiple lines, and it’s hard to follow the scoping
> > because of the ragged indentation, so use {} around this.
> 
> Done.
> 
>
https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
> File src/common/linux/tests/crash_generator.cc (right):
> 
>
https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
> src/common/linux/tests/crash_generator.cc:265: fprintf(stderr, "%s test is
> skipped due to failure to crash a process\n",
> On 2015/02/02 17:12:32, Mark Mentovai wrote:
> > Maybe the other two “return false” cases shouldn’t, but don’t you think that
> > this case should still cause the test to fail?
> 
> I'm asking myself the same question as well. CreateChildCrash assumes it can
> setrlimit and have access to /proc, etc, which may also not be true on certain
> platforms. For example, a truncated core file may cause the tests to fail but
> it's not really breakpad's fault.

ptal

[Mark Mentovai]

https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
File src/common/linux/tests/crash_generator.cc (right):

https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
src/common/linux/tests/crash_generator.cc:265: fprintf(stderr, "%s test is
skipped due to failure to crash a process\n",
Ben Chan wrote:
> On 2015/02/02 17:12:32, Mark Mentovai wrote:
> > Maybe the other two “return false” cases shouldn’t, but don’t you think that
> > this case should still cause the test to fail?
> 
> I'm asking myself the same question as well. CreateChildCrash assumes it can
> setrlimit and have access to /proc, etc, which may also not be true on certain
> platforms. For example, a truncated core file may cause the tests to fail but
> it's not really breakpad's fault.

Yes, but we’ve got heuristics to detect reasons that an unusable core might be
produced, presumably because we understand this. If we find that we’re failing
the test on a platform where things don’t match our expectations, we’d want to
adjust our heuristics. Not having this be a failure means that practically, this
won’t ever happen, and we’ll assume that everything’s OK when in fact nothing
has been tested at all. It would be better for a new platform to require someone
to look at this (by failing) than to silently succeed without any noticeable
signal.

[Ben Chan]

On 2015/02/02 22:27:45, Mark Mentovai wrote:
>
https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
> File src/common/linux/tests/crash_generator.cc (right):
> 
>
https://breakpad.appspot.com/4714002/diff/1/src/common/linux/tests/crash_gene...
> src/common/linux/tests/crash_generator.cc:265: fprintf(stderr, "%s test is
> skipped due to failure to crash a process\n",
> Ben Chan wrote:
> > On 2015/02/02 17:12:32, Mark Mentovai wrote:
> > > Maybe the other two “return false” cases shouldn’t, but don’t you think
that
> > > this case should still cause the test to fail?
> > 
> > I'm asking myself the same question as well. CreateChildCrash assumes it can
> > setrlimit and have access to /proc, etc, which may also not be true on
certain
> > platforms. For example, a truncated core file may cause the tests to fail
but
> > it's not really breakpad's fault.
> 
> Yes, but we’ve got heuristics to detect reasons that an unusable core might be
> produced, presumably because we understand this. If we find that we’re failing
> the test on a platform where things don’t match our expectations, we’d want to
> adjust our heuristics. Not having this be a failure means that practically,
this
> won’t ever happen, and we’ll assume that everything’s OK when in fact nothing
> has been tested at all. It would be better for a new platform to require
someone
> to look at this (by failing) than to silently succeed without any noticeable
> signal.

that makes sense. I guess CreateCoreDumpForTest could return an enum, with value
being one of {OK, FAILED, IGNORED} instead of bool.  Then a test can
ASSERT_NE(FAILED, crash_generator.CreateCoreDumpForTest(..)).  What do you
think?

[Mark Mentovai]

Yes, that sounds good.