MD Settings: split browser tests per-file

MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/56339d033d3d86cbdb2e8d19e8e4a5f4493fffa7
Cr-Commit-Position: refs/heads/master@{#399860}

[Dan Beam, 2016-06-09 01:36:32]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-06-13 16:14:11]

some numbers here:

monolithic test (before this CL): ~3100ms

checkbox: ~2000ms
dropdown: ~2400ms
prefUtil: ~1900ms
prefs: ~2200ms

overall time to repeat all CrSettings* tests:
before this CL: 4m 7s
after this CL: 4m 48s

note: don't use the actual measurements too much, they'll vary, just use these
numbers relatively

[dpapad, 2016-06-13 18:19:46]

https://codereview.chromium.org/2050053002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2050053002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:30:
'fake_settings_private.js',
Are those two files needed by the majority of CrSettingsBrowserTest subclasses?
If not, can we simply move those extra libraries in their respective subclasses
instead?

https://codereview.chromium.org/2050053002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:69: 
Since we split this to a separate C++ test fixture, how about specifying
browserPreload here and remove it from
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/dropdown....

https://codereview.chromium.org/2050053002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:85: 
Since we split this to a separate C++ test fixture, how about specifying
browserPreload here and remove it from
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/pref_uti....

[Dan Beam, 2016-06-14 00:23:09]

Description was changed from

==========
MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
==========

to

==========
MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
==========

[Dan Beam, 2016-06-14 00:23:30]

whoops, /cc michaelpg@, see https://codereview.chromium.org/2050053002/#msg3

[michaelpg, 2016-06-14 00:30:20]

michaelpg@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-06-14 00:30:21]

https://codereview.chromium.org/2050053002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2050053002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:69: 
On 2016/06/13 18:19:46, dpapad wrote:
> Since we split this to a separate C++ test fixture, how about specifying
> browserPreload here and remove it from
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/dropdown....

can't remember why i didn't just import i18n_setup.html in
settings_dropdown_menu.html. If that works, +1

[michaelpg, 2016-06-14 00:49:50]

On 2016/06/13 16:14:11, Dan Beam wrote:
> some numbers here:
> 
> monolithic test (before this CL): ~3100ms
> 
> checkbox: ~2000ms
> dropdown: ~2400ms
> prefUtil: ~1900ms
> prefs: ~2200ms

My theory: Running an empty test takes 1800ms.

The tests cases add some time:
* checkbox: 200ms
* dropdown: 600ms
* prefUtil: 100ms
* prefs:    400ms

Total for the test cases: 1300ms

Thus, running the monolithic test takes (1800 + 1300) = 3100ms

If you're worried about timeouts, I totally get isolating the slow tests
(dropdown, prefs).

I don't really have a problem with isolating *everything* if that's easier. It's
just that nobody has acknowledged that we probably only *need* to isolate the
tests that are *actually* slow, since the bulk of the time is outside of the
test-case-specific parts (loading settings-foo.html and testing it).

> 
> overall time to repeat all CrSettings* tests:
> before this CL: 4m 7s
> after this CL: 4m 48s
> 
> note: don't use the actual measurements too much, they'll vary, just use these
> numbers relatively

[Dan Beam, 2016-06-14 01:04:51]

On 2016/06/14 00:49:50, michaelpg wrote:
> On 2016/06/13 16:14:11, Dan Beam wrote:
> > some numbers here:
> > 
> > monolithic test (before this CL): ~3100ms
> > 
> > checkbox: ~2000ms
> > dropdown: ~2400ms
> > prefUtil: ~1900ms
> > prefs: ~2200ms
> 
> My theory: Running an empty test takes 1800ms.
> 
> The tests cases add some time:
> * checkbox: 200ms
> * dropdown: 600ms
> * prefUtil: 100ms
> * prefs:    400ms
> 
> Total for the test cases: 1300ms
> 
> Thus, running the monolithic test takes (1800 + 1300) = 3100ms
> 
> If you're worried about timeouts, I totally get isolating the slow tests
> (dropdown, prefs).

I am worried about timeouts.

Did you read https://codereview.chromium.org/2049943003#msg9 which shows which
parts of the test take time (spoiler: mocha.run() itself does not take long).

I get that it's probably fairly hard to read my jibberish.  Suggestions welcome
of different ways to represent/gather that data.

It was fairly hard to derive, as I added a crapton of logs in JS and in the
/generated/ C++ files 0_o.

> 
> I don't really have a problem with isolating *everything* if that's easier.
It's
> just that nobody has acknowledged that we probably only *need* to isolate the
> tests that are *actually* slow, since the bulk of the time is outside of the
> test-case-specific parts (loading settings-foo.html and testing it).
> 
> > 
> > overall time to repeat all CrSettings* tests:
> > before this CL: 4m 7s
> > after this CL: 4m 48s
> > 
> > note: don't use the actual measurements too much, they'll vary, just use
these
> > numbers relatively

[Dan Beam, 2016-06-14 01:22:56]

dbeam@chromium.org changed reviewers:
- dpapad@chromium.org

[Dan Beam, 2016-06-15 02:22:17]

Description was changed from

==========
MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
==========

to

==========
MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-06-15 02:24:16]

ok, this does a lot of the stuff asked for in review and over chat

[michaelpg, 2016-06-15 02:31:49]

lgtm

https://codereview.chromium.org/2050053002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2050053002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:25: browsePreload:
'about:inducebrowsercrashforrealz',
comment on the point of this

[Dan Beam, 2016-06-15 05:51:46]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-06-15 05:51:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2050053002/#ps70006
(title: "getter instead")

[commit-bot: I haz the power, 2016-06-15 05:52:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2050053002/70006

[Dan Beam, 2016-06-15 05:52:34]

https://codereview.chromium.org/2050053002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2050053002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:25: browsePreload:
'about:inducebrowsercrashforrealz',
On 2016/06/15 02:31:48, michaelpg wrote:
> comment on the point of this

Done.

[commit-bot: I haz the power, 2016-06-15 08:33:30]

Committed patchset #4 (id:70006)

[commit-bot: I haz the power, 2016-06-15 08:38:17]

Description was changed from

==========
MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: split browser tests per-file

Step 2 of splitting up CrSettingsBrowserTest into chunks

R=dpapad@chromium.org
BUG=534718
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/56339d033d3d86cbdb2e8d19e8e4a5f4493fffa7
Cr-Commit-Position: refs/heads/master@{#399860}
==========

[commit-bot: I haz the power, 2016-06-15 08:38:18]

Patchset 4 (id:??) landed as
https://crrev.com/56339d033d3d86cbdb2e8d19e8e4a5f4493fffa7
Cr-Commit-Position: refs/heads/master@{#399860}