No more foo-crash-expected.txt in layout test

[Yoichi Osato]

We have more than 1,000 layout tests to just confirm not crashed.

https://code.google.com/p/chromium/codesearch#search/&q=crash-expected.txt%20package:%5Echromium$%20file:%5Esrc/third_party/WebKit&type=cs&sq=package:chromium

A typical test is:

foo-crash.html:

if (window.testRunner) {

 testRunner.dumpAsText();

//…testing..

document.body = document.createElement(‘body’);

document.body.textContent = 'PASS if not crashed.';

foo-crash-expected.txt:

 PASS if not crashed.

In addition if we can’t set body textContext because of test case,

we sometimes need to use ‘iframe trick’ such as:

bar-crash.html:

if (window.testRunner) {

 testRunner.waitUntilDone();

<iframe src=bar-crash-body.html>

document.body = document.createElement(‘body’);

document.body.textContent = 'OK!';

and bar-crash-body.html and bar-crash-expected.txt

This bureaucracy is very annoying both to write and read.

Thus I propose 2 ways:

1. Add test behavior for *-crash.html.

• If run-webkit-test processes *-crash.html, it confirms just the file doesn’t cause crash

so that we don’t need -crash-expected.txt, textContent insertion and the iframe trick.

    2.  Add testRunner.finishWithoutCrash() function;

• the function outputs ‘OK’ so we can also remove textContent and the iframe trick and unify -crash-expected.txt format.

[Dominic Cooney]

I think it is great if our crash regression tests become more consistent. I believe there's no reason to upstream these to W3C, for example, so having them separate or easily identifiable would be good.

You shouldn't need to do anything special to confirm something doesn't crash. The test runner always reports crashes, right?

With regards to useless -expected.txt files, the W3 test harness is already configured to work a bit like this: Only tests which fail generate output, so passing tests have no -expected.txt files. (Just look at how few expectations the Service Worker Layout Tests have.) I'm not exactly sure how the mechanism works though.

 

    2.  Add testRunner.finishWithoutCrash() function;

• the function outputs ‘OK’ so we can also remove textContent and the iframe trick and unify -crash-expected.txt format.

I'm confused about why this is useful. Once the -expectations.txt files are gone, it doesn't matter if they would have ended in OK or not.

[Dirk Pranke]

run-webkit-tests checks to see if the output from content_shell matches the format we expect for a testharness-based test here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py&l=262

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/testharness_results.py&l=13

It should be straightforward to extend this code if you can define a convention for logging something to indicate that the test completed without crashing and that that's all that matters.

Or you could just define a naming convention for such tests.

-- Dirk

[Koji Ishii]

There should be a presubmit check too, so that upload does not require -expected when testharness.js is used.

I have a mild preference to Dirk's suggestion of a naming convention than a specialized method. I think we can save adding the method, save writing a call to it in every such test, and handles crash-on-close/quit case cleaner.

/koji

[Alan Cutter]

I'm in favour of just using testharness.js for crash tests to avoid -expected.txt files. It's consistent and less magical than having yet more file name based logic.

The crash test code itself is clean enough that we don't need a special function for it either.

test(function() { /* Do the thing. */ }, 'Don\'t crash when doing the thing.');

On Sat, 13 Jun 2015 at 20:26 'Koji Ishii' via blink-dev <blin...@chromium.org> wrote:

There should be a presubmit check too, so that upload does not require -expected when testharness.js is used.

Sometimes we want to check in -expected.txt for testharness.js tests when some of the tests fail (and we have bugs filed for them).

[Elliott Sprehn]

I'd rather we didn't use testharness.js, many crash tests require subtle or very specific actions to happen in a specific order and having this test runner doing lots of things around your code make it easy to have it not really test what you want.

I'd prefer we just use a special name.

[Julien Chaffraix]

On Sun, Jun 14, 2015 at 6:08 PM, Elliott Sprehn <esp...@chromium.org> wrote:
> I'd rather we didn't use testharness.js, many crash tests require subtle or
> very specific actions to happen in a specific order and having this test
> runner doing lots of things around your code make it easy to have it not
> really test what you want.
>
> I'd prefer we just use a special name.

The premise of this proposal is that it's annoying to have empty
-expected.txt files.

I don't think the premise is totally correct. It's important to have
self-documenting tests and as such we should dump more than just an OK
message. I strongly advise people to dump at least a description of
what is tested (and when appropriate a link to the bug too). Also a
subset of the crash tests are also validating and documenting our
behavior: we now don't crash so we are now exposing a new behavior to
the platform, that wasn't covered before.

Also it seems like the original example about the 'iframe trick'
(bar-crash-body.html) should be in a resources/ folder to avoid
generating expectations.

The original case alone doesn't seem like much pain to justify adding
some extra magic to our test harness (either with testharness.js or
using a magic postfix like -crash.html). I do like the postfix
-crash.html proposal though as it makes it clear what the test checks
but we don't need a strict rule or extra code for that.

Julien

[Christian Biesinger]

I'm not super-happy about never checking output for -crash tests.
Usually, it's not just important not to crash, but also behave
correctly, no? So the test should have an expectation that verifies
behavior.

Am I wrong? :)

-christian

[Yoichi Osato]

Thank you for many ideas and suggestions.

We need crash test cases to avoid regression so it is good if it is raw or ‘clean’ HTML style file without Blink harness because we can easily check

 that just run it in canary.

query-of-death HTML files are often tricky so adding code to output description and result inside

 the file gets more weird.

> You shouldn't need to do anything special to confirm something doesn't crash. The test runner always reports crashes, right?

Yes.

 > the W3 test harness is already configured to work a bit like this

Yes, testharness.js serves test API including crashness. However, not all query-of-death files can include <script src=testharness.js /> because it change Blink parser behavior.

> The premise of this proposal is that it's annoying to have empty

> -expected.txt files.

Plus, sometimes we need to write tricky code to output description and result inside

> I don't think the premise is totally correct. It's important to have

> self-documenting tests and as such we should dump more than just an OK

> message. I strongly advise people to dump at least a description of

> what is tested (and when appropriate a link to the bug too). Also a

> subset of the crash tests are also validating and documenting our

> behavior: we now don't crash so we are now exposing a new behavior to

> the platform, that wasn't covered before.

Those tests mainly confirm new behavior. All existing non-crash tests also confirm not crashed of course.

2015年6月16日(火) 5:14 Christian Biesinger <cbies...@chromium.org>:

[Noel Gordon]

On 17 June 2015 at 15:52, Yoichi Osato <yoi...@chromium.org> wrote:

> The premise of this proposal is that it's annoying to have empty

> -expected.txt files.

Plus, sometimes we need to write tricky code to output description and result inside

Why? A crash test either crashes, or it does not.  If it does not crash, the result is PASS.  I so no real need to write that to the output, but perhaps you have an example of why output is needed ...

~noel