Can you link to the original bug?

What do you mean by "paints pixels on the screen"? At what level was this broken? I'm concerned about the potential for flakiness in tests that inspect screenshots.

The bug is 905569. Hopefully we don't need to compare screenshots, just make sure something was painted. We have a unit test that covers this for the app-launcher (it uses ws::mojom::EnsureClientHasDrawnWindow()). I'm hoping we could use the exact same thing for keyboard shortcut viewer.

Do you want a Tast test or a unit test? (If you want to run a test binary compiled from the Chromium repository on real Chrome OS hardware, that's a thing that's pretty easy to do in Tast.)

What ever you recommend. The important thing is that the test runs on actual machines as the bug only occurred on real hardware. VMs may have shown the bug as well, but my understanding is only certain devices exhibited the behavior, so VMs would only show the bug if we have VMs configured similarly to the failing devices.

If the actual test code will be in C++ (which I'm guessing is the case, based on the ws::mojom::EnsureClientHasDrawnWindow() mention in #3), then you should probably:

a) add a new test target in the Chromium repo
b) add it to setup_test_lists in chromeos-chrome-9999.ebuild in chromiumos-overlay
c) update the chrome-binary-tests ebuild to install it
d) add a Tast test to run it (see security.SandboxLinuxUnittests as an example)

Can tast pull an existing test binary and use --gtest_filter? For example, I'm wondering if instead of (a) we could use browser_tests or unit_tests?

Hmm, those binaries are still around 100 MB each in my build dir after stripping. I don't think that we'd want to include them in test images just to get a single test.

Here are the sizes of the existing Chrome test binaries that are present on the random bob-paladin DUT that I SSH-ed to just now:

# ls -lah /usr/local/libexec/chrome-binary-tests/
total 40M
drwxr-xr-x. 2 root root 4.0K Nov 26 15:23 .
drwxr-xr-x. 5 root root 4.0K Nov 26 15:23 ..
-rwxr-xr-x. 1 root root 1.1M Nov 26 15:03 jpeg_decode_accelerator_unittest
-rwxr-xr-x. 1 root root 5.8M Nov 26 15:03 ozone_gl_unittests
-rwxr-xr-x. 1 root root 1.3M Nov 26 15:03 sandbox_linux_unittests
-rwxr-xr-x. 1 root root 6.0M Nov 26 15:03 video_decode_accelerator_unittest
-rwxr-xr-x. 1 root root 7.4M Nov 26 15:03 video_encode_accelerator_unittest
-rwxr-xr-x. 1 root root  18M Nov 26 15:03 wayland_client_perftests

In general, I'm weirded out to hear about Chrome tests that can catch bugs when run on real hardware that they can't on Linux devices. I also fear that we'll get unwanted behavior when running unit/browser tests on real hardware -- will they talk to real services over D-Bus instead of constructing their own stubs?

There is already some confusion about what tests should run where. I would personally to designate test suites as Linux only or CrOS Device only, although I realize that may not actually be the case today.

Creating a binary test requires some work. And it would not be too small since we need to pull in all the infra code needed to run ksv as a mojo app. And it could be on a different code path from chrome and not capturing real problems. 

So I would say we should create a tast test (like ui.ChromeLogin etc) and figure out a way to call ws::mojom::EnsureClientHasDrawnWindow from the tast.

Okay. Tast doesn't currently use Mojo anywhere, so you'll probably be blazing a new trail there.

Mojo does not have go support ATM. I vaguely remembered seening it before but could not no longer find it. I only see C/C++, JS, Java) now. Because of that, I don't see a good way to hook generic mojo support into tast. For now, I am thinking of extending autotestPrivate API to call ws::mojom::EnsureClientHasDrawnWindow and do it via tast_test_extension.

+rockot

The go support likely existed when mojo lived in github. I'm sure we dropped it when pulling mojo back into chrome.

Right, we removed it because there was no value in maintaining it. I would have no problem with someone landing go support if they also owned its support & maintenance, otherwise a one-off solution like #13 seems like a good idea.

drive-by note that to have caught the regression that prompted this bug, I don't think EnsureClientHasDrawnWindow would have been enough (client did draw the contents, the problem is that tiles wouldn't be displayed correctly), you'd really need an end-to-end test that captures the screen output.

I repro'd the problem by reverting the two CLs in  issue 905569 . My observation is that EnsureClientHasDrawnWindow never returns when the problem happens, that is WindowServerTestImpl::OnSurfaceActivated never gets called.

Scrape #17. I was using mojo wrongly and piman@ is correct. The current EnsureClientHasDrawnWindow impl does returns with success when the problem happens. So we would need to check the pixel output. I will extend EnsureClientHasDrawnWindow to do that.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3acfe10bc788469dc9e16f2bcb506abf16defb67

commit 3acfe10bc788469dc9e16f2bcb506abf16defb67
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Fri Jan 11 19:13:41 2019

ws: EnsureClientHasDrawnWindow sanity check output

Make WindowServerTestImpl::EnsureClientHasDrawnWindow capture
output of the first ClientRoot window of the client and sanity
check the captured pixels. The output is considered okay when
there are more than 5 colors.

Bug:  908545 
Change-Id: Iaa2306da6e2b40b0cf9a8379e71f564b0cf92a8b
Reviewed-on: https://chromium-review.googlesource.com/c/1406189
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622086}
[modify] https://crrev.com/3acfe10bc788469dc9e16f2bcb506abf16defb67/services/ws/BUILD.gn
[modify] https://crrev.com/3acfe10bc788469dc9e16f2bcb506abf16defb67/services/ws/window_server_test_impl.cc
[modify] https://crrev.com/3acfe10bc788469dc9e16f2bcb506abf16defb67/services/ws/window_server_test_impl.h
[modify] https://crrev.com/3acfe10bc788469dc9e16f2bcb506abf16defb67/services/ws/window_tree.cc
[modify] https://crrev.com/3acfe10bc788469dc9e16f2bcb506abf16defb67/services/ws/window_tree.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7335200afad80c820bc7ecc1f11ecc06a112b42e

commit 7335200afad80c820bc7ecc1f11ecc06a112b42e
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Fri Jan 11 19:15:13 2019

Add autotest API to check WS client output

- Add an ensureWindowServiceClientHasDrawnWindow method to check
  a Window Service client really paints on screen.
- Add a chrome_content_browser_manifest_test_overlay.json that is
  only applied with --use-test-config switch so that the autotest
  API could make a mojo call to WindowServerTest.

Bug:  908545 
Change-Id: I72f9b64fa06b4acf8e7fb8440dd65375949ef3c7
Reviewed-on: https://chromium-review.googlesource.com/c/1406155
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622088}
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/browser/OWNERS
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/browser/browser_resources.grd
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/browser/chrome_content_browser_client.cc
[add] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/browser/chrome_content_browser_manifest_test_overlay.json
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/chrome/common/extensions/api/autotest_private.idl
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/7335200afad80c820bc7ecc1f11ecc06a112b42e/tools/metrics/histograms/enums.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/9159886e01ace55d7737090de60f42621f5f5062

commit 9159886e01ace55d7737090de60f42621f5f5062
Author: Xiyuan Xia <xiyuan@google.com>
Date: Wed Jan 16 20:28:58 2019

tast-tests: Add KeyboardShortcutViewerPainting test

Add a test to check whether keyboard shortcut viewer (aka KSV)
really paints on screen. It also serves the purpose of verifying
a mojo app paints property via Window Service.

BUG= chromium:908545 
TEST=tast run $DUT ui.KeyboardShortcutViewerPainting

Change-Id: I5d57e0cf053acbb660e47cfa4564b7169e025cf1
Reviewed-on: https://chromium-review.googlesource.com/1406051
Commit-Ready: Xiyuan Xia <xiyuan@chromium.org>
Tested-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/9159886e01ace55d7737090de60f42621f5f5062/src/chromiumos/tast/local/bundles/cros/ui/keyboard_shortcut_viewer_painting.go