Support image / audio references in testharness.js tests.

Support image / audio references in testharness.js tests.

Asides from being the framework for Web Platform Tests, testharness.js
gets special treatment in our test suite. Specifically, we don't have to
commit -expected.txt files for testharness.js tests where we expect all the
assertions to pass. Unfortunately, this behavior does not apply to
testharness.js tests that generate images or audio files.

There is at least a good use case for testharness.js tests that also do
image comparions -- drag and drop tests that cover both DOM aspects and
the drag feedback image (a.k.a. ghost / preview). This CL adds support
for use cases like the one described above, by decoupling the
image/audio expectation checks from the text expectations checks. The
latter receive the special testharness.js treatment, while the former
are handled the same way as for non-testharness.js tests.

BUG=74302
Committed: https://crrev.com/9f5d7f5ddc14e5293117eecef0b59ab1ba9a5ca9
Cr-Commit-Position: refs/heads/master@{#432332}

[pwnall, 2016-11-03 15:51:14]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 15:51:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2016-11-03 16:37:16]

Description was changed from

==========
Support image / audio references in testharness.js tests.

BUG=74302
==========

to

==========
Support image / audio references in testharness.js tests.

Asides from being the framework for Web Platform Tests, testharness.js
gets special treatment in our test suite. Specifically, we don't have to
commit -expected.txt files for testharness.js tests where we expect all the
assertions to pass. Unfortunately, this behavior does not apply to
testharness.js tests that generate images or audio files.

There is at least a good use case for testharness.js tests that also do
image comparions -- drag and drop tests that cover both DOM aspects and
the drag feedback image (a.k.a. ghost / preview). This CL adds support
for use cases like the one described above, by decoupling the
image/audio expectation checks from the text expectations checks. The
latter receive the special testharness.js treatment, while the former
are handled the same way as for non-testharness.js tests.

BUG=74302
==========

[pwnall, 2016-11-03 16:40:40]

pwnall@chromium.org changed reviewers:
+ dpranke@chromium.org

[pwnall, 2016-11-03 16:40:40]

Can you please take a look?

If it helps, the CL I need this for is http://crrev.com/2441023003

Thank you very much!

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-11-03 19:15:30]

+qyearsley, +jeffcarp fyi ...

Generally speaking, we shouldn't be creating new pixel or audio tests if at all
possible (we should be writing c++ unit tests),
and if we do have to write new pixel tests, you shouldn't use testharness.js for
it; anything written using testharness.js
should be upstream-able to WPT (and, conversely, WPT tests can't be pixel
tests).

So, I don't think we should make this change. How hard would it be to change
your tests in the other CL to either be C++ tests or js-test tests?

[pwnall, 2016-11-03 20:23:18]

On 2016/11/03 19:15:30, Dirk Pranke wrote:
> +qyearsley, +jeffcarp fyi ...
> 
> Generally speaking, we shouldn't be creating new pixel or audio tests if at
all
> possible (we should be writing c++ unit tests),
> and if we do have to write new pixel tests, you shouldn't use testharness.js
for
> it; anything written using testharness.js
> should be upstream-able to WPT (and, conversely, WPT tests can't be pixel
> tests).
> 
> So, I don't think we should make this change. How hard would it be to change
> your tests in the other CL to either be C++ tests or js-test tests?

I wrote js-tests before, so I can definitely do that, it's not a problem :D
However, I'm having a hard time understanding why that's a better approach. Can
you please see my thought process and help me understand?

I thought that we'd want all future layout tests to use testharness.js, because
other developers are more likely to be familiar with the framework, and it does
a reasonable job of documenting expectations. The test I wrote will run in other
browsers, though it won't cover the aspect that requires the image dump (the
drag feedback image). Last but not least, the test can be run as a manual test,
by loading the file a the browser, giving other browser developers a quick way
to check their implementations against the code.

Given the relatively little structure we have around LayoutTests, I think the
benefits listed above outweigh the cost that the test's header may confuse
developers into thinking it's a WPT. Can you please shed more light here?

Thank you very much!

[Dirk Pranke, 2016-11-03 20:37:14]

These are good questions. The answers aren't obvious, and it's even possible
that I'm giving out bad advice.

As I said above, the first rule of thumb is if you're testing something that can
and should work the same in
other browsers, then you should be writing a test that will be upstreamed to
WPT. That does imply using
testharness.js, but the WPT has a strong bias (correctly, IMO) against things
that require manual review.

So, they don't generally want to add things to testharness.js that aren't
portable or might encourage
manual tests.

If the test isn't meant to be upstreamed, then these days you should generally
be writing a c++ unit
test instead; they're usually faster and less flaky.

Only if you can't do one of the above two should you be writing a layout test
that requires image or
audio output. 

In that case, it's less clear whether you can use testharness.js or not, but our
rule of thumb so far
has been that you shouldn't use testharness.js, in order to avoid confusion.

It's possible that this last paragraph is bad advice, though, and maybe it would
be better to encourage
testharness.js even for these sorts of tests. However, if we wanted to do this,
we should decide to do
so as a team, so I'd suggest having that conversation on blink-dev@.

Does that all make sense?

[pwnall, 2016-11-04 19:17:34]

On 2016/11/03 20:37:14, Dirk Pranke wrote:
> These are good questions. The answers aren't obvious, and it's even possible
> that I'm giving out bad advice.
> 
> As I said above, the first rule of thumb is if you're testing something that
can
> and should work the same in
> other browsers, then you should be writing a test that will be upstreamed to
> WPT. That does imply using
> testharness.js, but the WPT has a strong bias (correctly, IMO) against things
> that require manual review.
> 
> So, they don't generally want to add things to testharness.js that aren't
> portable or might encourage
> manual tests.
> 
> If the test isn't meant to be upstreamed, then these days you should generally
> be writing a c++ unit
> test instead; they're usually faster and less flaky.
> 
> Only if you can't do one of the above two should you be writing a layout test
> that requires image or
> audio output. 
> 
> In that case, it's less clear whether you can use testharness.js or not, but
our
> rule of thumb so far
> has been that you shouldn't use testharness.js, in order to avoid confusion.
> 
> It's possible that this last paragraph is bad advice, though, and maybe it
would
> be better to encourage
> testharness.js even for these sorts of tests. However, if we wanted to do
this,
> we should decide to do
> so as a team, so I'd suggest having that conversation on blink-dev@.
> 
> Does that all make sense?

I'm terribly sorry, I forgot to upgrade this thread!

What you said makes perfect sense. Thank you very much for explaining! Would you
like me to close this CL while blink-dev converges on a decision, so it goes off
of your review inbox? Or is it OK to leave it open?

[Dirk Pranke, 2016-11-04 19:27:54]

On 2016/11/04 19:17:34, pwnall wrote:
> What you said makes perfect sense. Thank you very much for explaining! Would
you
> like me to close this CL while blink-dev converges on a decision, so it goes
off
> of your review inbox? Or is it OK to leave it open?

It's fine to leave it open. Do whichever you prefer.

[pwnall, 2016-11-15 02:44:15]

On 2016/11/04 19:27:54, Dirk Pranke wrote:
> On 2016/11/04 19:17:34, pwnall wrote:
> > What you said makes perfect sense. Thank you very much for explaining! Would
> you
> > like me to close this CL while blink-dev converges on a decision, so it goes
> off
> > of your review inbox? Or is it OK to leave it open?
> 
> It's fine to leave it open. Do whichever you prefer.

In light of your comment on blink-dev, will you please take another look at this
CL?

[Dirk Pranke, 2016-11-15 23:17:59]

Yup, lgtm, thanks (and thanks for bearing with me).

qyearsley / jeffcarp - note the change.

[pwnall, 2016-11-15 23:22:57]

On 2016/11/15 23:17:59, Dirk Pranke wrote:
> Yup, lgtm, thanks (and thanks for bearing with me).
> 
> qyearsley / jeffcarp - note the change.

Thank _you_ for helping me do things the right way!

[pwnall, 2016-11-15 23:23:03]

The CQ bit was checked by pwnall@chromium.org

[commit-bot: I haz the power, 2016-11-15 23:23:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qyearsley, 2016-11-15 23:39:34]

Follow-up question:

 I didn't actually understand how testharness.js tests can produce image/audio
results; are there currently any examples of such tests?

[pwnall, 2016-11-15 23:43:13]

On 2016/11/15 23:39:34, qyearsley wrote:
> Follow-up question:
> 
>  I didn't actually understand how testharness.js tests can produce image/audio
> results; are there currently any examples of such tests?

The following CL (which is marked as depending on this one) will create such a
test.
http://crrev.com/2441023003

Without the change in this CL, it would be very hard to produce such a test,
because the test runner used by the CQ would demand text expectations, and the
presubmit check would bounce the said text expectations, assuming that all the
test cases pass.

[commit-bot: I haz the power, 2016-11-15 23:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 23:46:48]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pwnall, 2016-11-16 01:04:21]

The CQ bit was checked by pwnall@chromium.org

[commit-bot: I haz the power, 2016-11-16 01:04:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 01:20:43]

Description was changed from

==========
Support image / audio references in testharness.js tests.

Asides from being the framework for Web Platform Tests, testharness.js
gets special treatment in our test suite. Specifically, we don't have to
commit -expected.txt files for testharness.js tests where we expect all the
assertions to pass. Unfortunately, this behavior does not apply to
testharness.js tests that generate images or audio files.

There is at least a good use case for testharness.js tests that also do
image comparions -- drag and drop tests that cover both DOM aspects and
the drag feedback image (a.k.a. ghost / preview). This CL adds support
for use cases like the one described above, by decoupling the
image/audio expectation checks from the text expectations checks. The
latter receive the special testharness.js treatment, while the former
are handled the same way as for non-testharness.js tests.

BUG=74302
==========

to

==========
Support image / audio references in testharness.js tests.

Asides from being the framework for Web Platform Tests, testharness.js
gets special treatment in our test suite. Specifically, we don't have to
commit -expected.txt files for testharness.js tests where we expect all the
assertions to pass. Unfortunately, this behavior does not apply to
testharness.js tests that generate images or audio files.

There is at least a good use case for testharness.js tests that also do
image comparions -- drag and drop tests that cover both DOM aspects and
the drag feedback image (a.k.a. ghost / preview). This CL adds support
for use cases like the one described above, by decoupling the
image/audio expectation checks from the text expectations checks. The
latter receive the special testharness.js treatment, while the former
are handled the same way as for non-testharness.js tests.

BUG=74302
==========

[commit-bot: I haz the power, 2016-11-16 01:20:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-16 01:25:49]

Description was changed from

==========
Support image / audio references in testharness.js tests.

Asides from being the framework for Web Platform Tests, testharness.js
gets special treatment in our test suite. Specifically, we don't have to
commit -expected.txt files for testharness.js tests where we expect all the
assertions to pass. Unfortunately, this behavior does not apply to
testharness.js tests that generate images or audio files.

There is at least a good use case for testharness.js tests that also do
image comparions -- drag and drop tests that cover both DOM aspects and
the drag feedback image (a.k.a. ghost / preview). This CL adds support
for use cases like the one described above, by decoupling the
image/audio expectation checks from the text expectations checks. The
latter receive the special testharness.js treatment, while the former
are handled the same way as for non-testharness.js tests.

BUG=74302
==========

to

==========
Support image / audio references in testharness.js tests.

Asides from being the framework for Web Platform Tests, testharness.js
gets special treatment in our test suite. Specifically, we don't have to
commit -expected.txt files for testharness.js tests where we expect all the
assertions to pass. Unfortunately, this behavior does not apply to
testharness.js tests that generate images or audio files.

There is at least a good use case for testharness.js tests that also do
image comparions -- drag and drop tests that cover both DOM aspects and
the drag feedback image (a.k.a. ghost / preview). This CL adds support
for use cases like the one described above, by decoupling the
image/audio expectation checks from the text expectations checks. The
latter receive the special testharness.js treatment, while the former
are handled the same way as for non-testharness.js tests.

BUG=74302

Committed: https://crrev.com/9f5d7f5ddc14e5293117eecef0b59ab1ba9a5ca9
Cr-Commit-Position: refs/heads/master@{#432332}
==========

[commit-bot: I haz the power, 2016-11-16 01:25:50]

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