Test Accelerators In Interactive UI Tests in CrOs (Retry)

Test Accelerators In Interactive UI Tests (Retry)

This is a retry CL after making these tests ChromeOS-only tests and disabling them in the official build.
Original CL: https://codereview.chromium.org/1026513005/

In order to avoid future regressions on ChromeOS.

BUG=469235
TEST=interactive_ui_tests --gtest_filter=AcceleratorInteractiveUITest.*

Committed: https://crrev.com/c325afc591b830f15382902f9eaa82e905dc2bbe
Cr-Commit-Position: refs/heads/master@{#322245}

[afakhry, 2015-03-25 19:37:27]

afakhry@chromium.org changed reviewers:
+ jamescook@chromium.org

[afakhry, 2015-03-25 19:38:10]

James, please review, thanks!

[James Cook, 2015-03-25 20:14:54]

Another nit: I would title the CL something like "Reland: cros: Add an
interactive_ui_test for keyboard accelerators".

The current title kinda sounds like you are moving an existing test into
interactive_ui_tests. Also, we usually say "reland" for these sorts of patches.

Thanks for writing tests for this -- I think it will help us a lot in the
future.

https://codereview.chromium.org/1005093004/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_interactive_uitest.cc (right):

https://codereview.chromium.org/1005093004/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_interactive_uitest.cc:125: #if defined(OS_CHROMEOS)
&& !defined(OFFICIAL_BUILD)
Now that I think about it more I would just rename this file to ..._chromeos.cc
and take out all the #if defined(OS_CHROMEOS) checks.

Also, I would use the MAYBE_ pattern for official builds -- I think we still
want to compile it on the official builders, just not run it.

https://codereview.chromium.org/1005093004/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1005093004/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1081:
'../ash/accelerators/accelerator_interactive_uitest.cc',
I would rename this accelerator_interactive_uitest_chromeos.cc and put it in the
section above.

[afakhry, 2015-03-25 20:46:52]

https://codereview.chromium.org/1005093004/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_interactive_uitest.cc (right):

https://codereview.chromium.org/1005093004/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_interactive_uitest.cc:125: #if defined(OS_CHROMEOS)
&& !defined(OFFICIAL_BUILD)
On 2015/03/25 20:14:54, James Cook wrote:
> Now that I think about it more I would just rename this file to
..._chromeos.cc
> and take out all the #if defined(OS_CHROMEOS) checks.
> 
> Also, I would use the MAYBE_ pattern for official builds -- I think we still
> want to compile it on the official builders, just not run it.

Done.

https://codereview.chromium.org/1005093004/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1005093004/diff/1/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1081:
'../ash/accelerators/accelerator_interactive_uitest.cc',
On 2015/03/25 20:14:54, James Cook wrote:
> I would rename this accelerator_interactive_uitest_chromeos.cc and put it in
the
> section above.

Done.

[James Cook, 2015-03-25 20:53:03]

LGTM. Please update the CL description also.

[afakhry, 2015-03-25 20:59:23]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-03-25 20:59:54]

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

[commit-bot: I haz the power, 2015-03-25 22:13:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-25 22:14:55]

Patchset 2 (id:??) landed as
https://crrev.com/c325afc591b830f15382902f9eaa82e905dc2bbe
Cr-Commit-Position: refs/heads/master@{#322245}