Refactor multiprocess test code to allow multiple child processes to be launched.

Refactor multiprocess test code to allow multiple child processes to be launched.

BUG=
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/263582c2d06789d1373bfbfdaed1135cf60cd4ef

[erikwright (departed), 2015-06-01 20:41:22]

erikwright@chromium.org changed reviewers:
+ scottmg@chromium.org

[erikwright (departed), 2015-06-01 20:41:23]

Scott: PTAL.

[scottmg, 2015-06-01 21:53:22]

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
File test/win/win_child_process.cc (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
test/win/win_child_process.cc:162: if (!SetHandleInformation(
Can you move this inside the helper function too? (And then move the explanatory
comment too)

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
File test/win/win_child_process_test.cc (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
test/win/win_child_process_test.cc:41: }
Could you add a test that launches multiple child processes, since that's the
motivation for the change?

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
test/win/win_multiprocess.h:46: scoped_ptr<WinChildProcess::Handles>
child_handles = WinChildProcess::Launch();
80 col

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
test/win/win_multiprocess.h:140: virtual void
extra \n

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
File test/win/win_multiprocess_test.cc (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
test/win/win_multiprocess_test.cc:41: }
A few tests aren't here, just in need a rebase?

[erikwright (departed), 2015-06-24 20:01:35]

Scott, PTAL.

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
File test/win/win_child_process.cc (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
test/win/win_child_process.cc:162: if (!SetHandleInformation(
On 2015/06/01 21:53:22, scottmg wrote:
> Can you move this inside the helper function too? (And then move the
explanatory
> comment too)

Done.

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
File test/win/win_child_process_test.cc (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_child_proc...
test/win/win_child_process_test.cc:41: }
On 2015/06/01 21:53:22, scottmg wrote:
> Could you add a test that launches multiple child processes, since that's the
> motivation for the change?

Done.

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
File test/win/win_multiprocess.h (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
test/win/win_multiprocess.h:46: scoped_ptr<WinChildProcess::Handles>
child_handles = WinChildProcess::Launch();
On 2015/06/01 21:53:22, scottmg wrote:
> 80 col

Done.

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
test/win/win_multiprocess.h:140: virtual void
On 2015/06/01 21:53:22, scottmg wrote:
> extra \n

Done.

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
File test/win/win_multiprocess_test.cc (right):

https://codereview.chromium.org/1164453003/diff/40001/test/win/win_multiproce...
test/win/win_multiprocess_test.cc:41: }
On 2015/06/01 21:53:22, scottmg wrote:
> A few tests aren't here, just in need a rebase?

Done.

[scottmg, 2015-06-25 04:13:25]

Looks good, glad you got back to this. Just a few comments.

https://codereview.chromium.org/1164453003/diff/100001/snapshot/win/pe_image_...
File snapshot/win/pe_image_annotations_reader_test.cc (right):

https://codereview.chromium.org/1164453003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_annotations_reader_test.cc:47: template <TestType
test_type>
It's unusual to use lower case here because it looks surprising in the body. The
normal name would be TestType for the value, but since that conflicts with the
enum, maybe just 'Type'.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
File test/win/win_child_process.cc (right):

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.cc:135: (*read_handle) = temp_read.Pass();
Remove unnecessary parens

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
File test/win/win_child_process.h (right):

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.h:54: // TODO(erikwright) force the client to
subclass ChildProcess. Otherwise
It's going to have to implement Run(), right? I don't understand what this
comment is trying to explain.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.h:56: // TODO(erikwright): Use some form of static
assert that gives a clear
This second TODO isn't really actionable, so just remove it.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.h:59: exit(0);
The existing children (mostly?) do an exit(something) already. I wonder if
should instead make that part of the contract, remove the exit(0) and add a
ADD_FAILURE() here?

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_multiproc...
File test/win/win_multiprocess_test.cc (right):

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_multiproc...
test/win/win_multiprocess_test.cc:24: template <int exit_code>
exit_code -> ExitCode

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_multiproc...
test/win/win_multiprocess_test.cc:46: template <FailureType failure_type>
similar

[erikwright (departed), 2015-06-25 14:36:02]

PTAL.

https://codereview.chromium.org/1164453003/diff/100001/snapshot/win/pe_image_...
File snapshot/win/pe_image_annotations_reader_test.cc (right):

https://codereview.chromium.org/1164453003/diff/100001/snapshot/win/pe_image_...
snapshot/win/pe_image_annotations_reader_test.cc:47: template <TestType
test_type>
On 2015/06/25 04:13:24, scottmg wrote:
> It's unusual to use lower case here because it looks surprising in the body.
The
> normal name would be TestType for the value, but since that conflicts with the
> enum, maybe just 'Type'.

Done.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
File test/win/win_child_process.cc (right):

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.cc:135: (*read_handle) = temp_read.Pass();
On 2015/06/25 04:13:24, scottmg wrote:
> Remove unnecessary parens

Done.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
File test/win/win_child_process.h (right):

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.h:54: // TODO(erikwright) force the client to
subclass ChildProcess. Otherwise
On 2015/06/25 04:13:24, scottmg wrote:
> It's going to have to implement Run(), right? I don't understand what this
> comment is trying to explain.

The TODO was out-of-date, sorry.

The point is that, given that this is a static method templated on T, the only
thing this method really requires is that T declare a method Run() whose return
value may be assigned to an int.

The static_cast adds an additional, important constraint.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.h:56: // TODO(erikwright): Use some form of static
assert that gives a clear
On 2015/06/25 04:13:24, scottmg wrote:
> This second TODO isn't really actionable, so just remove it.

Done.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_child_pro...
test/win/win_child_process.h:59: exit(0);
On 2015/06/25 04:13:24, scottmg wrote:
> The existing children (mostly?) do an exit(something) already. I wonder if
> should instead make that part of the contract, remove the exit(0) and add a
> ADD_FAILURE() here?

I would be more inclined to have Run() return an int (the exit code).

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_multiproc...
File test/win/win_multiprocess_test.cc (right):

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_multiproc...
test/win/win_multiprocess_test.cc:24: template <int exit_code>
On 2015/06/25 04:13:25, scottmg wrote:
> exit_code -> ExitCode

Done.

https://codereview.chromium.org/1164453003/diff/100001/test/win/win_multiproc...
test/win/win_multiprocess_test.cc:46: template <FailureType failure_type>
On 2015/06/25 04:13:25, scottmg wrote:
> similar

In the end I thought a non-template approach might be less obscure.

[scottmg, 2015-06-25 15:48:37]

lgtm

[erikwright (departed), 2015-07-31 16:32:03]

Committed patchset #9 (id:160001) manually as
263582c2d06789d1373bfbfdaed1135cf60cd4ef (presubmit successful).