Re: Request for review/approval of reflection tests

On Tue, Mar 8, 2011 at 4:44 PM, Ms2ger <ms2ger@gmail.com> wrote:
> I haven't yet done an exhaustive review, but I'd like to note that I believe
> that the test for area@shape is incorrect, as the spec doesn't limit that
> attribute to only known values (AFAICT).

Thanks, fixed:

http://dvcs.w3.org/hg/html/rev/b25280f5b50a

> As for reviewing your test, I find it pretty hard to follow. Having to
> follow the chain element->attribute->type->reflectsFoo function->reflects
> function->doReflects function makes it easy to lose track what you're
> reviewing.

Well, there are really two parts to it: the code and the data.  The
data format is simple to follow and easy to check.  E.g., in
elements-text.js:

    "a": [
        // Conforming
        "target", "rel", "media", "hreflang", "type", "rel", "relList",
        // Obsolete
        "coords", "charset", "name", "rev", "shape",
    ],
    "em": [],

This just says that "a" has those twelve reflected attributes (in
addition to global ones), while "em" has no special reflected
attributes.  Each of those attributes is defined in the attribs array
in reflection.js, unless it's just a string, and the format there is
pretty easy to follow too, e.g.:

    "acceptCharset": ["string", "accept-charset"],
    "action": "url",
    "formAction": "url",
    "audio": "settable tokenlist",
    "autocomplete": ["enum", "autocomplete", {"values": ["on", "off"],
"missing": "on"}],

I assume this part isn't a problem.  Checking that it's correct is
just a tedious exercise in cross-checking with the spec, unless there
are bugs in how the data format is interpreted.

Once the data is verified, the code of doReflects() is what does the
real work.  Granted, this is going to be much more complicated than
most test suites, but it's not practical to be this thorough on so
many different elements and attributes without some complexity.  This
test suite is a program in its own right, I think necessarily.

If you have suggestions for some way I could modify the tests to make
them easier to review, such as clarifying or simplifying some parts of
it or even rewriting it from scratch, please say so.  Otherwise I'm
not sure what I can do.

> Also, there's quite a bit of code with comments suggesting an update after
> the rewrite you did; it might be useful to fix those.

I only noticed two of those.  I got rid of the wrapper functions:

http://dvcs.w3.org/hg/html/rev/e172a6f9e78e

This also makes the data format more directly translate into the
arguments of reflects(), so it's easier to understand if you already
understand reflects().  (And I fixed a bug where some input.form*
attributes weren't being tested by mistake.)

Received on Wednesday, 9 March 2011 00:49:16 UTC