Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Paramaterize the MouseLockSilentAfterTargetUnlock interactive_ui_test
since it is currently run on the Mac bots and its logic needs to change
for the simplified UI mode.

Other tests will be treated similarly in a follow-up for better
automated coverage (http://crbug.com/584136).

BUG=585009, 584136
Committed: https://crrev.com/6fc638c272aec7271700f6e36675b9a2bc21fcfa
Cr-Commit-Position: refs/heads/master@{#378327}

[tapted, 2016-02-18 06:00:45]

tapted@chromium.org changed reviewers:
+ asvitkine@chromium.org

[tapted, 2016-02-18 06:00:46]

Hi Alexei, PTAL. I think this is the next step before submitting cl/114291338

[Alexei Svitkine (slow), 2016-02-18 16:22:13]

Description was changed from

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

BUG=585009
==========

to

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

BUG=585009
==========

[Alexei Svitkine (slow), 2016-02-18 16:22:13]

asvitkine@chromium.org changed reviewers:
+ isherman@chromium.org
- asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-02-18 16:22:28]

Over to Ilya

[Ilya Sherman, 2016-02-18 22:12:29]

https://codereview.chromium.org/1709803004/diff/1/testing/variations/fieldtri...
File testing/variations/fieldtrial_testing_config_mac.json (right):

https://codereview.chromium.org/1709803004/diff/1/testing/variations/fieldtri...
testing/variations/fieldtrial_testing_config_mac.json:237: "group_name":
"Enabled"
You probably need to list enable_features here.

https://codereview.chromium.org/1709803004/diff/1/testing/variations/fieldtri...
testing/variations/fieldtrial_testing_config_mac.json:241: }
No need to list the control group -- just the non-default group.

[tapted, 2016-02-23 04:28:37]

https://codereview.chromium.org/1709803004/diff/1/testing/variations/fieldtri...
File testing/variations/fieldtrial_testing_config_mac.json (right):

https://codereview.chromium.org/1709803004/diff/1/testing/variations/fieldtri...
testing/variations/fieldtrial_testing_config_mac.json:237: "group_name":
"Enabled"
On 2016/02/18 22:12:29, Ilya Sherman wrote:
> You probably need to list enable_features here.

Ah! I thought it looked too easy :). Done.

https://codereview.chromium.org/1709803004/diff/1/testing/variations/fieldtri...
testing/variations/fieldtrial_testing_config_mac.json:241: }
On 2016/02/18 22:12:29, Ilya Sherman wrote:
> No need to list the control group -- just the non-default group.

Done.

[Ilya Sherman, 2016-02-23 07:08:31]

Syntactically LG, but it looks like you have a relevant test failure.

[tapted, 2016-02-23 07:09:27]

On 2016/02/23 07:08:31, Ilya Sherman wrote:
> Syntactically LG, but it looks like you have a relevant test failure.

yup! working on a fix now - I think i'm going to paramaterize the test class so
it can test both, but I'll need another reviewer.

[tapted, 2016-02-23 07:29:44]

Description was changed from

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

BUG=585009
==========

to

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Paramaterize the MouseLockSilentAfterTargetUnlock interactive_ui_test
since it is currently run on the Mac bots and its logic needs to change
for the simplified UI mode.

Other tests will be treated similarly in a follow-up for better
automated coverage (http://crbug.com/584136).

BUG=585009, 584136
==========

[tapted, 2016-02-23 11:05:21]

tapted@chromium.org changed reviewers:
+ scheib@chromium.org

[tapted, 2016-02-23 11:05:22]

Hi Vincent, please take a look for review/OWNERS in
fullscreen_controller_interactive_browsertest.cc

(Ilya PTAL for the test config and to ensure this approach for testing both UI
types meshes with how the fieldtrial testing does its thing)

[scheib, 2016-02-23 18:46:01]

LGTM

[Ilya Sherman, 2016-02-24 01:31:19]

https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
File
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc
(right):

https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc:112:
};
I'm not sure that any of this is necessary.  I think the only needed change is
the ShouldPrompt() check within the test, based on the current command-line
flags.  Everything else should be being configured by the test harness.

[tapted, 2016-02-24 04:03:42]

On 2016/02/24 01:31:19, Ilya Sherman wrote:
>
https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
> File
>
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
>
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc:112:
> };
> I'm not sure that any of this is necessary.  I think the only needed change is
> the ShouldPrompt() check within the test, based on the current command-line
> flags.  Everything else should be being configured by the test harness.

Maybe it's not essential, but ideally we would be able retain trybot test
coverage of both paths, which I think requires the test to be run twice? (hence
parameterization).

I did some spelunking to see how fieldtrial_testing_config_mac.json gets plumbed
in. I think it will be fed in via the call to
`chrome_variations::AssociateDefaultFieldTrialConfig(feature_list.get());` in
chrome_browser_main.cc. That iterates through all the groups, doing

base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(group.study,
group.group_name);

(CreateFieldTrial returns nullptr if the same study is specified a second time
with a different group_name, so I see that listing both `Enabled` and `Disabled`
the way I had originally won't achieve much.)

But then for a given build with FIELDTRIAL_TESTING_ENABLED #defined, I don't see
a way for it to execute the test again, without the feature enabled. It looks
like it will only happen if the test is run with a GOOGLE_CHROME_BUILD (or
fieldtrial_testing_like_official_build), which might be annoyingly late to pick
up a regression. (Also, in that case, would it contact Finch to do the usual
automatic-group-selection/distribution when running the test, so it could be
~random which path is tested?)

It's entirely possible there's a secret test harness feature that does exactly
what I'm after, which I've totally missed though :)

[tapted, 2016-02-24 06:01:35]

On 2016/02/24 04:03:42, tapted wrote:
> On 2016/02/24 01:31:19, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
> > File
> >
>
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
> >
>
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc:112:
> > };
> > I'm not sure that any of this is necessary.  I think the only needed change
is
> > the ShouldPrompt() check within the test, based on the current command-line
> > flags.  Everything else should be being configured by the test harness.
> 
> Maybe it's not essential, but ideally we would be able retain trybot test
> coverage of both paths, which I think requires the test to be run twice?
(hence
> parameterization).
> 
> I did some spelunking to see how fieldtrial_testing_config_mac.json gets
plumbed
> in. I think it will be fed in via the call to
> `chrome_variations::AssociateDefaultFieldTrialConfig(feature_list.get());` in
> chrome_browser_main.cc. That iterates through all the groups, doing
> 
> base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(group.study,
> group.group_name);
> 
> (CreateFieldTrial returns nullptr if the same study is specified a second time
> with a different group_name, so I see that listing both `Enabled` and
`Disabled`
> the way I had originally won't achieve much.)
> 
> But then for a given build with FIELDTRIAL_TESTING_ENABLED #defined, I don't
see
> a way for it to execute the test again, without the feature enabled. It looks
> like it will only happen if the test is run with a GOOGLE_CHROME_BUILD (or
> fieldtrial_testing_like_official_build), which might be annoyingly late to
pick
> up a regression. (Also, in that case, would it contact Finch to do the usual
> automatic-group-selection/distribution when running the test, so it could be
> ~random which path is tested?)

As a concrete example, I would expect a CL like
https://codereview.chromium.org/1732563004/ to fail the CQ -- the old-style UI
doesn't seem to be tested, so if new code introduces a regression in the old UI,
then I don't think the CQ would pick it up.

[Ilya Sherman, 2016-02-24 06:23:46]

isherman@chromium.org changed reviewers:
+ danduong@chromium.org

[Ilya Sherman, 2016-02-24 06:23:47]

On 2016/02/24 06:01:35, tapted wrote:
> On 2016/02/24 04:03:42, tapted wrote:
> > On 2016/02/24 01:31:19, Ilya Sherman wrote:
> > >
> >
>
https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
> > > File
> > >
> >
>
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1709803004/diff/40001/chrome/browser/ui/exclu...
> > >
> >
>
chrome/browser/ui/exclusive_access/fullscreen_controller_interactive_browsertest.cc:112:
> > > };
> > > I'm not sure that any of this is necessary.  I think the only needed
change
> is
> > > the ShouldPrompt() check within the test, based on the current
command-line
> > > flags.  Everything else should be being configured by the test harness.
> > 
> > Maybe it's not essential, but ideally we would be able retain trybot test
> > coverage of both paths, which I think requires the test to be run twice?
> (hence
> > parameterization).
> > 
> > I did some spelunking to see how fieldtrial_testing_config_mac.json gets
> plumbed
> > in. I think it will be fed in via the call to
> > `chrome_variations::AssociateDefaultFieldTrialConfig(feature_list.get());`
in
> > chrome_browser_main.cc. That iterates through all the groups, doing
> > 
> > base::FieldTrial* trial =
base::FieldTrialList::CreateFieldTrial(group.study,
> > group.group_name);
> > 
> > (CreateFieldTrial returns nullptr if the same study is specified a second
time
> > with a different group_name, so I see that listing both `Enabled` and
> `Disabled`
> > the way I had originally won't achieve much.)
> > 
> > But then for a given build with FIELDTRIAL_TESTING_ENABLED #defined, I don't
> see
> > a way for it to execute the test again, without the feature enabled. It
looks
> > like it will only happen if the test is run with a GOOGLE_CHROME_BUILD (or
> > fieldtrial_testing_like_official_build), which might be annoyingly late to
> pick
> > up a regression. (Also, in that case, would it contact Finch to do the usual
> > automatic-group-selection/distribution when running the test, so it could be
> > ~random which path is tested?)
> 
> As a concrete example, I would expect a CL like
> https://codereview.chromium.org/1732563004/ to fail the CQ -- the old-style UI
> doesn't seem to be tested, so if new code introduces a regression in the old
UI,
> then I don't think the CQ would pick it up.

You're right that if you want both paths to be tested, the default test harness
doesn't provide that.  That was basically a compromise to reduce resource usage
impacts from adding test coverage for field trial variants.  I guess
parametrizing the specific test that you're most interested in is a reasonable
way to get the best of both worlds.

I'd like Dan to take a quick look, since he's a lot more familiar with the
rationale for how we set up the field trial testing overall, than I am.  But,
LGTM assuming LG to Dan =)

[tapted, 2016-02-25 22:59:33]

danduong: ping?

[danduong, 2016-02-29 22:47:03]

On 2016/02/25 at 22:59:33, tapted wrote:
> danduong: ping?

Yup. LGTM. Apologize for the delay.

[tapted, 2016-02-29 22:48:25]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-02-29 22:48:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709803004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709803004/40001

[commit-bot: I haz the power, 2016-03-01 00:03:13]

Description was changed from

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Paramaterize the MouseLockSilentAfterTargetUnlock interactive_ui_test
since it is currently run on the Mac bots and its logic needs to change
for the simplified UI mode.

Other tests will be treated similarly in a follow-up for better
automated coverage (http://crbug.com/584136).

BUG=585009, 584136
==========

to

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Paramaterize the MouseLockSilentAfterTargetUnlock interactive_ui_test
since it is currently run on the Mac bots and its logic needs to change
for the simplified UI mode.

Other tests will be treated similarly in a follow-up for better
automated coverage (http://crbug.com/584136).

BUG=585009, 584136
==========

[commit-bot: I haz the power, 2016-03-01 00:03:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 00:04:55]

Description was changed from

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Paramaterize the MouseLockSilentAfterTargetUnlock interactive_ui_test
since it is currently run on the Mac bots and its logic needs to change
for the simplified UI mode.

Other tests will be treated similarly in a follow-up for better
automated coverage (http://crbug.com/584136).

BUG=585009, 584136
==========

to

==========
Add ViewsSimplifiedFullscreenUI to the fieldtrial testing config on Mac

Paramaterize the MouseLockSilentAfterTargetUnlock interactive_ui_test
since it is currently run on the Mac bots and its logic needs to change
for the simplified UI mode.

Other tests will be treated similarly in a follow-up for better
automated coverage (http://crbug.com/584136).

BUG=585009, 584136

Committed: https://crrev.com/6fc638c272aec7271700f6e36675b9a2bc21fcfa
Cr-Commit-Position: refs/heads/master@{#378327}
==========

[commit-bot: I haz the power, 2016-03-01 00:04:56]

Patchset 3 (id:??) landed as
https://crrev.com/6fc638c272aec7271700f6e36675b9a2bc21fcfa
Cr-Commit-Position: refs/heads/master@{#378327}