Move testEvictedTabReloadFailure to an external URL egtest target.

Move testEvictedTabReloadFailure to an external URL egtest target.

BUG=709126
R=baxley@chromium.org

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

[lpromero, 2017-04-25 22:01:53]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 22:03:30]

Dry run: 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-04-25 23:15:53]

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

[commit-bot: I haz the power, 2017-04-25 23:15:54]

Dry run: This issue passed the CQ dry run.

[lpromero, 2017-04-26 21:30:39]

Forgot to send…

[baxley, 2017-05-01 23:20:06]

LGTM, once a few high-level questions are addressed. Thanks for the refactor!

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/BUILD.gn (right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/BUILD.gn:180: # is unavailable. Reference to
crbug.com/694662.
Is the bug reference necessary? If it is, should it be a TODO?

When this is fixed, will the bug number still be useful in the comment?

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/BUILD.gn:181: source_set("external_url_eg_tests") {
Is there enough in common that it's worth creating test_support for the
intersection between the external URL and hermetic tests? Or are they unique
enough (or this will be removed at some point) that it's better to do it this
way?

Just a question, I trust your judgement, so if you feel one way you don't need
to wait for me.

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm
(right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm:26: // Test
for the TabUsageRecorder class.
Update comment to mention external URL tests for TabUsageRecorder?

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/tab_usage_recorder_test_util.mm (right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/tab_usage_recorder_test_util.mm:5: #import
"ios/chrome/browser/metrics/tab_usage_recorder_test_util.h"
#include? (I mess this up a lot, so it may be correct :))

[lpromero, 2017-05-02 13:55:39]

The CQ bit was checked by lpromero@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 13:55:53]

Dry run: CQ is trying da patch.

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

[lpromero, 2017-05-02 13:56:01]

I'll wait for your feedback on the second thread of comments.

Thanks!

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/BUILD.gn (right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/BUILD.gn:180: # is unavailable. Reference to
crbug.com/694662.
On 2017/05/01 23:20:06, baxley wrote:
> Is the bug reference necessary? If it is, should it be a TODO?
> 
> When this is fixed, will the bug number still be useful in the comment?

When crbug.com/694662 is fixed, these tests won't rely on network anymore, and
could be folded back to the main EG target. I'll update the comment.

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/BUILD.gn:181: source_set("external_url_eg_tests") {
On 2017/05/01 23:20:06, baxley wrote:
> Is there enough in common that it's worth creating test_support for the
> intersection between the external URL and hermetic tests? Or are they unique
> enough (or this will be removed at some point) that it's better to do it this
> way?

Personally, I think we could remove this external_url_eg_tests target once the
underlying bug (crbug.com/694662) is fixed.
Below, I created a test_support that is the intersection between the external
URL and hermetic tests. Did you envision something broader than tab usage
recorder tests?

> Just a question, I trust your judgement, so if you feel one way you don't need
> to wait for me.

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm
(right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/external_url_tab_usage_recorder_egtest.mm:26: // Test
for the TabUsageRecorder class.
On 2017/05/01 23:20:06, baxley wrote:
> Update comment to mention external URL tests for TabUsageRecorder?

Done.

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/tab_usage_recorder_test_util.mm (right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/tab_usage_recorder_test_util.mm:5: #import
"ios/chrome/browser/metrics/tab_usage_recorder_test_util.h"
On 2017/05/01 23:20:06, baxley wrote:
> #include? (I mess this up a lot, so it may be correct :))

Done. Thanks!

[baxley, 2017-05-02 14:18:46]

LGTM

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/BUILD.gn (right):

https://codereview.chromium.org/2836403002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/BUILD.gn:181: source_set("external_url_eg_tests") {
On 2017/05/02 13:56:01, lpromero wrote:
> On 2017/05/01 23:20:06, baxley wrote:
> > Is there enough in common that it's worth creating test_support for the
> > intersection between the external URL and hermetic tests? Or are they unique
> > enough (or this will be removed at some point) that it's better to do it
this
> > way?
> 
> Personally, I think we could remove this external_url_eg_tests target once the
> underlying bug (crbug.com/694662) is fixed.
> Below, I created a test_support that is the intersection between the external
> URL and hermetic tests. Did you envision something broader than tab usage
> recorder tests?
> 
> > Just a question, I trust your judgement, so if you feel one way you don't
need
> > to wait for me.
> 

No, I meant just for the tab usage recorder tests. I think it's good how you
have it. Especially if these won't live forever.

[lpromero, 2017-05-02 14:20:27]

The CQ bit was unchecked by lpromero@chromium.org

[lpromero, 2017-05-02 14:20:34]

The CQ bit was checked by lpromero@chromium.org

[commit-bot: I haz the power, 2017-05-02 14:20:55]

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-05-02 15:27:47]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493734834104930,
"parent_rev": "4d000d1bfbbf3965f8c13f968fb6847a9c27c221", "commit_rev":
"eb7020961d6e218330eef899e33aef7a865219ae"}

[commit-bot: I haz the power, 2017-05-02 15:28:06]

Description was changed from

==========
Move testEvictedTabReloadFailure to an external URL egtest target.

BUG=709126
R=baxley@chromium.org
==========

to

==========
Move testEvictedTabReloadFailure to an external URL egtest target.

BUG=709126
R=baxley@chromium.org

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

[commit-bot: I haz the power, 2017-05-02 15:28:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/eb7020961d6e218330eef899e33a...