Add test for CHECK exit code behavior.

Add test for CHECK exit code behavior.

BUG=687326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2697423002
Cr-Commit-Position: refs/heads/master@{#452197}
Committed: https://chromium.googlesource.com/chromium/src/+/c0f16eacc2e54b2a6c0c3a4f6f2766f59d582827

[Will Harris, 2017-02-16 23:41:57]

Description was changed from

==========
Add test for CHECK exit code behavior.

BUG=687326
==========

to

==========
Add test for CHECK exit code behavior.

BUG=687326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Will Harris, 2017-02-17 01:21:51]

Description was changed from

==========
Add test for CHECK exit code behavior.

BUG=687326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add test for CHECK exit code behavior.

BUG=687326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Will Harris, 2017-02-17 01:21:52]

wfh@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org

[Will Harris, 2017-02-17 01:24:50]

no rush on this, I know jam@ is OOO

PTAL

isherman -> chrome/browser/metrics/metrics_service_browsertest.cc
jam -> content/

[Ilya Sherman, 2017-02-18 01:25:04]

Thanks!  Mostly LG; just have some comments on trying to find a good balance
between test clarity/simplicity and code maintainability.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:171: observer.Wait();
Could you please create a small utility function for navigating to a crash URL
and waiting for the crash?  It's used in three tests, and all the boilerplate
doesn't really help with test understandability, so we might as well make the
test structure simpler.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:182: }
Please also include this in the above function, or extract out a separate little
utility for it.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:190: // 4. chrome://crash
Perhaps we could document the first three page loads within the OpenTabs()
documentation, and simplify this documentation to something like

"Expect four page loads: three from OpenTabs(), plus chrome://crash"

WDYT?  Also, would be good to include something like this documentation in the
test below.

[jam, 2017-02-21 16:03:16]

lgtm

[Will Harris, 2017-02-21 21:53:00]

Patchset #3 (id:40001) has been deleted

[Will Harris, 2017-02-21 21:54:43]

addressed code review comments; isherman@ -> PTAL

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:171: observer.Wait();
On 2017/02/18 01:25:04, Ilya Sherman wrote:
> Could you please create a small utility function for navigating to a crash URL
> and waiting for the crash?  It's used in three tests, and all the boilerplate
> doesn't really help with test understandability, so we might as well make the
> test structure simpler.

Done.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:182: }
On 2017/02/18 01:25:04, Ilya Sherman wrote:
> Please also include this in the above function, or extract out a separate
little
> utility for it.

Done.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:190: // 4. chrome://crash
On 2017/02/18 01:25:04, Ilya Sherman wrote:
> Perhaps we could document the first three page loads within the OpenTabs()
> documentation, and simplify this documentation to something like
> 
> "Expect four page loads: three from OpenTabs(), plus chrome://crash"
> 
> WDYT?  Also, would be good to include something like this documentation in the
> test below.

Done.

[Ilya Sherman, 2017-02-21 22:50:34]

Sorry for being difficult, but could you please split up the extracted code into
several functions, as I suggested?  Basically I'd like to preserve the test
structure of arrange, act, and assert, as described at go/unit-test-practices
under the "Structure" section.

[Will Harris, 2017-02-21 23:05:18]

done. PTAL.

[Ilya Sherman, 2017-02-21 23:18:00]

Thanks, Will, LGTM.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:148: // exits... it's not
clear to me how to test that.
Is this TODO obsolete?  If not, please preserve it for at least one of the
tests.

https://codereview.chromium.org/2697423002/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://codereview.chromium.org/2697423002/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:205: // One tab should
crash.
Optional nit: This comment feels superfluous to the code -- unlike the comment
above, which explains why the constant is "4" (as opposed to some other number
of navigations), this one feels like it's just documenting what I'd expect the
code to mean anyway.  That said, if you think it adds clarify, feel free to keep
it here.

[Will Harris, 2017-02-21 23:43:37]

I moved the comment to the top of the test since it seems to apply to all tests
and not just the one it was originally placed into.

I won't commit until I hear that's okay from you.

Thanks for the quick turnaround on reviews.

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://codereview.chromium.org/2697423002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:148: // exits... it's not
clear to me how to test that.
On 2017/02/21 23:17:59, Ilya Sherman wrote:
> Is this TODO obsolete?  If not, please preserve it for at least one of the
> tests.

this comment probably applies to all the tests here, so I moved it up to the top
of the test, where it makes most sense.

FWIW I'm also not sure how to test this, since it requires the browser to exit
and then reload...

https://codereview.chromium.org/2697423002/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_browsertest.cc (right):

https://codereview.chromium.org/2697423002/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_browsertest.cc:205: // One tab should
crash.
On 2017/02/21 23:18:00, Ilya Sherman wrote:
> Optional nit: This comment feels superfluous to the code -- unlike the comment
> above, which explains why the constant is "4" (as opposed to some other number
> of navigations), this one feels like it's just documenting what I'd expect the
> code to mean anyway.  That said, if you think it adds clarify, feel free to
keep
> it here.

sure I removed it.

[Ilya Sherman, 2017-02-22 01:19:07]

LGTM, thanks.

[Will Harris, 2017-02-22 17:57:39]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2017-02-22 17:57:40]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2697423002/#ps100001
(title: "rejig comments")

[commit-bot: I haz the power, 2017-02-22 17:58:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 18:19:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:19:54]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Will Harris, 2017-02-22 18:21:02]

android_arm64_dbg_recipe failure is unrelated build failure.

[Will Harris, 2017-02-22 19:01:13]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2017-02-22 19:02:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 21:05:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487790073752020,
"parent_rev": "aedc88255e58fb8c4db948c47efa11ddea24b43c", "commit_rev":
"c0f16eacc2e54b2a6c0c3a4f6f2766f59d582827"}

[commit-bot: I haz the power, 2017-02-22 21:06:29]

Description was changed from

==========
Add test for CHECK exit code behavior.

BUG=687326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add test for CHECK exit code behavior.

BUG=687326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2697423002
Cr-Commit-Position: refs/heads/master@{#452197}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0f16eacc2e54b2a6c0c3a4f6f27...
==========

[commit-bot: I haz the power, 2017-02-22 21:06:33]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c0f16eacc2e54b2a6c0c3a4f6f27...