[webkit-dev] Simplifying syntax in test_expectations.txt (bug 86691)

[Dirk Pranke]

There's lot of good discussion going on in this thread ... I'm going
to attempt to reply to various threads in one message, hixie-style :)

On Thu, May 17, 2012 at 1:06 AM, Maciej Stachowiak <mjs at apple.com> wrote:
> SKIP and WONTFIX seem parallel to PASS to me.

[and]

On Thu, May 17, 2012 at 11:07 AM, Maciej Stachowiak <mjs at apple.com> wrote:
> To me, it seems like all the current "modifiers" except for the platform limitations are actually expected outcomes.

"SKIP" and "SLOW" are in some ways as much instructions as outcomes.
That said, I'm not sure making this distinction is worth the cost. The
bug URLs and "WONTFIX" as annotations, not outcomes (although perhaps
distinguishing WONTFIX separately also isn't worth the cost).


On Thu, May 17, 2012 at 9:40 AM, Darin Adler <darin at apple.com> wrote:
> Seriously, syntax is a significant barrier. Having to know which special characters to use. I don’t see this “clear delineation” you speak of. Just special punctuation I have to use to satisfy the computer.
>

Syntax is a double-edged sword, to be certain. There's clearly no need
to keep "=" and ":" around just to satisfy the computer, i.e., the
grammar doesn't need them to disambiguate or anything. However, I --
and clearly at least some others -- find tokens mixed together to be
harder to read.

There are other techniques we could use to increase readability.
Maciej suggests column alignment, which is perhaps kinda fragile but
does have some advantages.

On Wed, May 16, 2012 at 10:37 PM, Benjamin Poulain <benjamin at webkit.org> wrote:
> I would go for something verbose in a easy to understand format. E.g.
> { test: 'fast/html/keygen.html',
>  platform: 'mac, linux',
>  bug: 'webkit.org/b/66666',
>  expectedResults: ['crash', 'text', 'image'],
>  action: 'skip' }
>

This is very easy to understand. Unfortunately, it's also verbose as
heck, and, at least in chromium's case, we have hundreds of tests
marked as WONTFIX or SKIP, so this would be very hard to navigate
unless you put each expectation in a single (long) line, at which
point the readability suffers, I think.

On Thu, May 17, 2012 at 10:06 AM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> Unfortunately, "image+text" is different is "image text". The former
> indicates that the test will have mismatches in both .txt and .png files
> while the latter indicates that either .txt or .png mismatches (i.e. the
> test is flaky in terms of which failure it causes).
>
> I personally think this is extremely confusing, and we should get rid of
> this distinction altogether, and treat "Image Text" like both "image text"
> and "image+text" but I'll defer that to a separate discussion.
>

In a world where failures live in the file for an extended period of
time (i.e., how the chromium port currently operates), I would not be
a fan of this. In a world where failures that persist beyond a short
time are checked in (e.g., as foo-failing.txt or whatever), I can see
the argument that having anything more than a FAIL suffix is unneeded.

> I'd say we should rename text, image, and text+image to TextFailure,
> ImageFailure, TextAndImageFailures for now to clear the confusion.

Please, no. "Failure" is basically implied and your suggestion to me
makes things more verbose and harder to read.

Now, as for the more comprehensive comments:

On Wed, May 16, 2012 at 10:47 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>  how about something like
> webkit.org/12356 [Win Mac] [Text] failing-test.html
> <rniwa> [Linux] [Skip] temporarily-skipped-test.html
> Here, platforms and expectations are wrapped around [] and non-URL comments
> are wrapped in <>. (I think we should just use IRC nick).

Something like this isn't too bad, and ...

On Thu, May 17, 2012 at 4:30 AM, Ojan Vafai <ojan at chromium.org> wrote:
> I have a proposal that hopefully addresses everyone's concerns, is minimally
> different from the current format *and* unifies the format with Skipped
> lists (i.e. Skipped lists as they exist today are valid
> test_expectations.txt format).

I like this :)

>The changes from the current format are as follows:
> -Leaving out any "outcomes" means the test is skipped, unless the test has a
> SLOW modifier, in which case the implied outcome is PASS.
> -Remove the SKIP modifier entirely.

ok.

> -Make everything but the test name case-insensitive.

I don't think I like this; it could lead to a lot of arbitrarily
different formatting in the file, making things harder to read. I
think we'd probably find ourselves (re-)converging on some convention
pretty quickly. I personally find all uppercase fairly easy to read in
this case since it distinguishes the modifiers from the test name
(like Peter points out). If we have some other clear delimiter this
would probably be less important, in which case all lower case would
be fine as well. Initial caps seems less good to me.

> -Have the test name be the last item on the line

this seems reasonable. I have wondered (along with Darin) if it would
make more sense to put the test name first (and then sort the file by
test?), but I haven't really come up with a syntax I prefer.

> -Separate modifiers/outcomes/testname with a common delimiter (i.e. ":")
> -Any line starting with // or # is a comment

I assume supporting two different kinds of comments is intended to get
syntax compatibility for both expectations files (which currently use
//) and Skipped files (which use #). A noble goal, but this just seems
like it's unnecessary complexity in the long run and we should just
pick one or the other and update the other set of files to match.

> -Including a bug entry is optional (maybe only if the test is skipped or
> wontfix?)

I'm not sure I like this idea ... it seems like it's a good thing to
have some place to go to find the rationale for why an entry exists,
without having to look through the version history.

> -Bugs are listed as URLs, except for the bug_ojan format.

I continue to find this unfortunately verbose, but I can be convinced
it's enough of a win to be worth it.

Also, at some point an "All" modifier came into being; I would not
think that it's necessary.

-- Dirk