Made Pixel tests functional. Captures tab content and compares to ref image

Made Pixel tests functional. Captures tab content and compares to ref image

BUG=278398
R=dtu@chromium.org, kbr@chromium.org, piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228950

[bajones, 2013-09-04 18:46:51]

There are still some decisions to be made re: this code. Primarily, it currently
outputs the reference/failure images into a fixed location, while the existing
pixel test harness accepts a command line argument from the bots to determine
where the output should go. It's now clear what the best way to communicate that
will be in this environment. I'm uploading the code now to better facilitate
that discussion.

Although @dtu and @nduca are not owners of these files I would like a LG from
one of them before committing to ensure we're following Telemetry best
practices.

@piman: If you could LG the additional comments on the pixel tests I'd
appreciate it.

[piman, 2013-09-04 19:31:24]

LGTM for the comments in the tests, but see additional suggestion.

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/page_sets/pi...
File content/test/gpu/page_sets/pixel_tests.json (right):

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/page_sets/pi...
content/test/gpu/page_sets/pixel_tests.json:9: { "action": "wait", "seconds": 2
}
Rather than wait an arbitrary number of seconds (and risk flakiness), could we
wait for a particular event coming from the page instead?

[bajones, 2013-09-04 20:12:00]

On 2013/09/04 19:31:24, piman wrote:
> Rather than wait an arbitrary number of seconds (and risk flakiness), could we
> wait for a particular event coming from the page instead?

We do that with the WebGL conformance tests, so in theory yes. There's a
question of whether or not we can ensure that painting is done by the time we
observe the event/capture the tab, but I think that's work that's already being
done in a different CL.

[Zhenyao Mo, 2013-09-05 00:23:13]

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/gpu_tests/pi...
File content/test/gpu/gpu_tests/pixel.py (right):

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/gpu_tests/pi...
content/test/gpu/gpu_tests/pixel.py:13: test_data_dir =
os.path.abspath(os.path.join(
The reason we use a passed in dir is because the src folder will be blown away
from time to time (hard clobber), so you don't want to put your ref images
there.  Put it somewhere more stable.

[Ken Russell (switch to Gerrit), 2013-09-05 00:27:04]

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/gpu_tests/pi...
File content/test/gpu/gpu_tests/pixel.py (right):

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/gpu_tests/pi...
content/test/gpu/gpu_tests/pixel.py:17: ref_image_dir =
os.path.join(test_data_dir, 'gpu_reference')
The current harness is careful not to dump images into the source tree,
presumably to avoid collisions with any newly created files. Shouldn't the new
harness do the same?

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/page_sets/pi...
File content/test/gpu/page_sets/pixel_tests.json (right):

https://codereview.chromium.org/23478021/diff/1/content/test/gpu/page_sets/pi...
content/test/gpu/page_sets/pixel_tests.json:9: { "action": "wait", "seconds": 2
}
On 2013/09/04 19:31:24, piman wrote:
> Rather than wait an arbitrary number of seconds (and risk flakiness), could we
> wait for a particular event coming from the page instead?

Agree 100%. While frame-synced snapshots are in development (Issue 256848),
these tests ought to be able to issue something like three no-op
requestAnimationFrame callbacks, after which the results should be guaranteed to
be on screen. Each test should import some JavaScript helper file to implement
this, and then that one file can be changed to change the tests' exit
conditions.

[bajones, 2013-09-05 18:42:41]

Small update. Telemetry now waits for the tests to indicate that they are done
before validating. Should be less potentially flaky than the timeout.

This is done by injecting a mock domAutomationController, which the tests were
already attempting to use anyway to signal their completion. This should allow
the tests to be used by both the old and new harnesses during any transitional
period.

[Ken Russell (switch to Gerrit), 2013-09-06 00:35:35]

https://codereview.chromium.org/23478021/diff/10001/content/test/gpu/gpu_test...
File content/test/gpu/gpu_tests/pixel.py (right):

https://codereview.chromium.org/23478021/diff/10001/content/test/gpu/gpu_test...
content/test/gpu/gpu_tests/pixel.py:18: ref_image_dir =
os.path.join(test_data_dir, 'gpu_reference')
These directories really need to be passed in from the outside. Could you follow
the pattern of the --generated-dir and --reference-dir arguments in the previous
harness in
http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDIA...
?

[bajones, 2013-09-06 17:19:13]

On 2013/09/06 00:35:35, Ken Russell wrote:
> These directories really need to be passed in from the outside. Could you
follow
> the pattern of the --generated-dir and --reference-dir arguments in the
previous
> harness in
>
http://build.chromium.org/p/chromium.gpu/builders/Linux%2520Release%2520%2528...
> ?

Yes, I agree. For the record, the defaults that I have in place right now
actually match what's in the previous harness if you don't specify a command
line override.
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...)
but obviously that's not appropriate for use on the bots.

My first instinct would be to expose the same command line args that the
existing test uses for consistency, but there's currently no way to expose new
command line args with the telemetry test runner. Attempting to pass anything
that hasn't been explicitly allowed causes the test harness to gives you "error:
no such option: --foo". The page test classes have an overrideable method,
AddCommandLineOptions, that says in the comments that it can be used to expose
new command line args but in test_runner tests this method is called well after
the command line validation that blocks unknown options and as such doesn't help
this situation. I believe @dtu said this method was intended for use by
run_measurements, and he seemed to feel that allowing arbitrary command line
args to be passed to tests like this one would harm reproducability.

(Slight tangent: I'd also like to expose a command line arg for selecting the
version of the WebGL conformance tests to run, but that falls into the same
category as above)

@dtu's recommendation is that the images resulting from the pixel tests be
attached to the results object that's passed into the ValidatePage method. That
sounds fine to me, but after some initial investigation I'm still not clear on
how the results would be communicated to the bots through that object. The bot
still needs to indicate the destination directories somehow, and command line
args are still the most effective way I can think of to do that.

[Ken Russell (switch to Gerrit), 2013-09-06 21:16:55]

On 2013/09/06 17:19:13, bajones wrote:
> On 2013/09/06 00:35:35, Ken Russell wrote:
> > These directories really need to be passed in from the outside. Could you
> follow
> > the pattern of the --generated-dir and --reference-dir arguments in the
> previous
> > harness in
> >
>
http://build.chromium.org/p/chromium.gpu/builders/Linux%252520Release%252520%...
> > ?
> 
> Yes, I agree. For the record, the defaults that I have in place right now
> actually match what's in the previous harness if you don't specify a command
> line override.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...)
> but obviously that's not appropriate for use on the bots.
> 
> My first instinct would be to expose the same command line args that the
> existing test uses for consistency, but there's currently no way to expose new
> command line args with the telemetry test runner. Attempting to pass anything
> that hasn't been explicitly allowed causes the test harness to gives you
"error:
> no such option: --foo". The page test classes have an overrideable method,
> AddCommandLineOptions, that says in the comments that it can be used to expose
> new command line args but in test_runner tests this method is called well
after
> the command line validation that blocks unknown options and as such doesn't
help
> this situation. I believe @dtu said this method was intended for use by
> run_measurements, and he seemed to feel that allowing arbitrary command line
> args to be passed to tests like this one would harm reproducability.
> 
> (Slight tangent: I'd also like to expose a command line arg for selecting the
> version of the WebGL conformance tests to run, but that falls into the same
> category as above)
> 
> @dtu's recommendation is that the images resulting from the pixel tests be
> attached to the results object that's passed into the ValidatePage method.
That
> sounds fine to me, but after some initial investigation I'm still not clear on
> how the results would be communicated to the bots through that object. The bot
> still needs to indicate the destination directories somehow, and command line
> args are still the most effective way I can think of to do that.

There is no question that the tests have to be configurable via command line
arguments. That is how the scripts on the bots work. The fact that the
test_runner tests don't allow custom command line arguments, but other kinds of
tests do, is a bug. Let's please proceed with fixing that issue.

I missed the fact that the default directories are the same in the old C++ test
harness and pixel.py. It's fine in that case to leave them as they are in
pixel.py, but the command line argument for configuring the directories is
necessary.

[dtu, 2013-09-10 00:20:50]

On 2013/09/06 21:16:55, Ken Russell wrote:
> On 2013/09/06 17:19:13, bajones wrote:
> > On 2013/09/06 00:35:35, Ken Russell wrote:
> > > These directories really need to be passed in from the outside. Could you
> > follow
> > > the pattern of the --generated-dir and --reference-dir arguments in the
> > previous
> > > harness in
> > >
> >
>
http://build.chromium.org/p/chromium.gpu/builders/Linux%25252520Release%25252...
> > > ?
> > 
> > Yes, I agree. For the record, the defaults that I have in place right now
> > actually match what's in the previous harness if you don't specify a command
> > line override.
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...)
> > but obviously that's not appropriate for use on the bots.
> > 
> > My first instinct would be to expose the same command line args that the
> > existing test uses for consistency, but there's currently no way to expose
new
> > command line args with the telemetry test runner. Attempting to pass
anything
> > that hasn't been explicitly allowed causes the test harness to gives you
> "error:
> > no such option: --foo". The page test classes have an overrideable method,
> > AddCommandLineOptions, that says in the comments that it can be used to
expose
> > new command line args but in test_runner tests this method is called well
> after
> > the command line validation that blocks unknown options and as such doesn't
> help
> > this situation. I believe @dtu said this method was intended for use by
> > run_measurements, and he seemed to feel that allowing arbitrary command line
> > args to be passed to tests like this one would harm reproducability.
> > 
> > (Slight tangent: I'd also like to expose a command line arg for selecting
the
> > version of the WebGL conformance tests to run, but that falls into the same
> > category as above)
> > 
> > @dtu's recommendation is that the images resulting from the pixel tests be
> > attached to the results object that's passed into the ValidatePage method.
> That
> > sounds fine to me, but after some initial investigation I'm still not clear
on
> > how the results would be communicated to the bots through that object. The
bot
> > still needs to indicate the destination directories somehow, and command
line
> > args are still the most effective way I can think of to do that.
> 
> There is no question that the tests have to be configurable via command line
> arguments. That is how the scripts on the bots work. The fact that the
> test_runner tests don't allow custom command line arguments, but other kinds
of
> tests do, is a bug. Let's please proceed with fixing that issue.
> 
> I missed the fact that the default directories are the same in the old C++
test
> harness and pixel.py. It's fine in that case to leave them as they are in
> pixel.py, but the command line argument for configuring the directories is
> necessary.

I talked with tonyg and we agreed that we can extend run_gpu_tests to allow
tests to have command-line options. run_benchmark will continue to disallow
them.

For the destination directories, TestResults objects are allowed to have
options, so if you add the images to the results object, you can expose
destination directory options there.

[bajones, 2013-10-15 23:15:21]

@dtu, @nduca, @kbr: PTAL

After much yak shaving (https://codereview.chromium.org/23694031/) this is
finally looking like it's ready to go in. Still need a couple of LGs, and then
we can focus on bot integration via the gpu recipe.

One change worth calling out is an added "Crop" function on PngBitmap. The
ability to capture just a portion of the tab should really be handled by the
Snapshot API, but this is an effective shortcut that allows the ticket to move
forward and gives an easy path to upgrade to the "right" answer later.

All other changes are just bringing this test closer inline to the original
gpu_pixel_browsertest.cc functionality. In particular, output filenames now
match the original to avoid problems with existing tools.

[bajones, 2013-10-15 23:15:55]

@dtu, @nduca, @kbr: PTAL

After much yak shaving (https://codereview.chromium.org/23694031/) this is
finally looking like it's ready to go in. Still need a couple of LGs, and then
we can focus on bot integration via the gpu recipe.

One change worth calling out is an added "Crop" function on PngBitmap. The
ability to capture just a portion of the tab should really be handled by the
Snapshot API, but this is an effective shortcut that allows the ticket to move
forward and gives an easy path to upgrade to the "right" answer later.

All other changes are just bringing this test closer inline to the original
gpu_pixel_browsertest.cc functionality. In particular, output filenames now
match the original to avoid problems with existing tools.

[Ken Russell (switch to Gerrit), 2013-10-15 23:31:15]

This looks very good overall but one issue and one comment.

https://codereview.chromium.org/23478021/diff/30001/content/test/gpu/gpu_test...
File content/test/gpu/gpu_tests/pixel.py (right):

https://codereview.chromium.org/23478021/diff/30001/content/test/gpu/gpu_test...
content/test/gpu/gpu_tests/pixel.py:16: os.path.dirname(__file__), '..', '..',
'data', 'gpu'))
It's necessary to be able to override this directory with a command line
argument.

https://codereview.chromium.org/23478021/diff/30001/content/test/gpu/gpu_test...
content/test/gpu/gpu_tests/pixel.py:132: capture_time = datetime.now()
Unused variable?

https://codereview.chromium.org/23478021/diff/30001/content/test/gpu/page_set...
File content/test/gpu/page_sets/pixel_tests.json (right):

https://codereview.chromium.org/23478021/diff/30001/content/test/gpu/page_set...
content/test/gpu/page_sets/pixel_tests.json:7: "url":
"file:///../../data/gpu/pixel_canvas2d.html",
I think dtu is in the process of landing a CL which replaces all file:/// URLs
with file:// .

[Ken Russell (switch to Gerrit), 2013-10-15 23:32:27]

+skyostil because the Android bot on the GPU waterfall is actually already
running these tests, so we have to be careful not to break it. See
src/build/android/buildbot/bb_device_steps.py.

[bajones, 2013-10-16 00:04:24]

On 2013/10/15 23:31:15, Ken Russell wrote:
> It's necessary to be able to override this directory with a command line
> argument.

Derp. I botched a local branch merge here. Thanks for catching it.

Updated.

[Ken Russell (switch to Gerrit), 2013-10-16 00:37:33]

LGTM with a couple of minor comments.

https://codereview.chromium.org/23478021/diff/38001/content/test/gpu/gpu_test...
File content/test/gpu/gpu_tests/pixel.py (right):

https://codereview.chromium.org/23478021/diff/38001/content/test/gpu/gpu_test...
content/test/gpu/gpu_tests/pixel.py:19: ref_image_dir =
os.path.join(test_data_dir, 'gpu_reference')
Could you rename these to something like "default_generated_data_dir" to be more
clear?

https://codereview.chromium.org/23478021/diff/38001/content/test/gpu/gpu_test...
content/test/gpu/gpu_tests/pixel.py:168: help='Overrides the default location
for reference images')
Can you use "default=" to get rid of the if-tests in GetReferenceImage and
WriteErrorImages?

[dtu, 2013-10-16 01:20:41]

lgtm

https://chromiumcodereview.appspot.com/23478021/diff/30001/content/test/gpu/p...
File content/test/gpu/page_sets/pixel_tests.json (right):

https://chromiumcodereview.appspot.com/23478021/diff/30001/content/test/gpu/p...
content/test/gpu/page_sets/pixel_tests.json:7: "url":
"file:///../../data/gpu/pixel_canvas2d.html",
On 2013/10/15 23:31:16, Ken Russell wrote:
> I think dtu is in the process of landing a CL which replaces all file:/// URLs
> with file:// .

Yup, https://chromiumcodereview.appspot.com/24451002/

[bajones, 2013-10-16 05:11:11]

On 2013/10/16 00:37:33, Ken Russell wrote:
> Could you rename these to something like "default_generated_data_dir" to be
more
> clear?
> 
> Can you use "default=" to get rid of the if-tests in GetReferenceImage and
> WriteErrorImages?

Done and done.

On 2013/10/16 01:20:41, dtu wrote:
> Yup, https://chromiumcodereview.appspot.com/24451002/

Awesome! CL has been updated to support that. I'll hold off landing till I can
test it against your changes.

[commit-bot: I haz the power, 2013-10-16 16:58:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/23478021/45001

[bajones, 2013-10-16 18:08:41]

Committed patchset #6 manually as r228950 (presubmit successful).

[bajones, 2013-10-16 18:10:28]

On 2013/10/16 18:08:41, bajones wrote:
> Committed patchset #6 manually as r228950 (presubmit successful).

CQ has gone mad. (See the ever increasing number of inactive
"chromium_presubmit" steps above). Landed manually. I'll be watching closely for
tree failures.