Bug 39985 - Several layout tests needlessly use invalid HTML
Summary: Several layout tests needlessly use invalid HTML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 39259
  Show dependency treegraph
 
Reported: 2010-06-01 02:22 PDT by Eric Seidel (no email)
Modified: 2010-06-02 14:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.82 KB, patch)
2010-06-01 02:27 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2010-06-01 23:44 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-06-01 02:22:17 PDT
Several layout tests needlessly use invalid HTML
Comment 1 Eric Seidel (no email) 2010-06-01 02:27:42 PDT
Created attachment 57525 [details]
Patch
Comment 2 Adam Barth 2010-06-01 10:29:09 PDT
Comment on attachment 57525 [details]
Patch

LayoutTests/ChangeLog:12
 +             for this test to depend on WebKit's old parser behavior.
Can you cite a specific test here?  I want to be sure we don't lose test coverage.

LayoutTests/ChangeLog:18
 +             no need to depend on the old title behavior here.
This is also tested in fast/tokenizer.  I worry that some of these changes will cause web compat problems.  We need to keep close tabs on this sort of thing.
Comment 3 Alexey Proskuryakov 2010-06-01 16:37:22 PDT
Comment on attachment 57525 [details]
Patch

Besides Web compatibility, there is compatibility with Mac applications using WebKit (such as Dashboard).

I think that there are enough different changes to regression tests here to discuss them as part of review, not post-review.
Comment 4 Adam Barth 2010-06-01 22:03:41 PDT
@ap: I don't understand your comment.  We're not changing any behavior.  We're just improving the quality of our test suite by testing parsing issues in parser tests and editing issues in editing tests.
Comment 5 Alexey Proskuryakov 2010-06-01 22:20:31 PDT
Adam, I think I'm saying the same as you did in your review. The ChangeLog should mention which tests cover the aspects of behavior that are no longer covered by the changed tests, and a reviewer needs to verify those claims.

I never said anything about changing behavior.
Comment 6 Adam Barth 2010-06-01 22:27:42 PDT
Ah, thanks.  I misunderstood your comment.
Comment 7 Eric Seidel (no email) 2010-06-01 23:44:34 PDT
Created attachment 57625 [details]
Patch
Comment 8 Alexey Proskuryakov 2010-06-02 00:37:09 PDT
I'm concerned that after this change, current behavior will mostly be tested as failure cases in html5lib test suite. It may be too easy to overlook that we'll be changing behavior. After all, HTML5 is nothing but a draft yet.

Maybe such tests should be copied to fast/invalid?
Comment 9 Adam Barth 2010-06-02 00:55:13 PDT
> Maybe such tests should be copied to fast/invalid?

The tests aren't invalid, as far as I can tell.  They appear to be perfectly good tests with typos.

> I'm concerned that after this change, current behavior will mostly be tested as failure cases in html5lib test suite. It may be too easy to overlook that we'll be changing behavior.

Every time we change the results of a test in html5lib we're changing behavior.  It's unclear to me why you think these particular parser behaviors ought to be tested in editing tests.

The <title> changes are also tested in fast/tokenizer by two tests.  The <foo<bar case is already tested in fast/invalid, which is the remedy you propose.

As far as I can tell, there's nothing we can do to address your concerns.

> After all, HTML5 is nothing but a draft yet.

Well, perhaps we should implement the HTML4 parser then.  :P
Comment 10 Adam Barth 2010-06-02 01:06:59 PDT
> > After all, HTML5 is nothing but a draft yet.
> 
> Well, perhaps we should implement the HTML4 parser then.  :P

Ok.  I thought of a more witty retort...  Let's try that again.  :)

> After all, HTML5 is nothing but a draft yet.

That's true, and, after all, evolution nothing but a theory.
Comment 11 WebKit Commit Bot 2010-06-02 03:45:54 PDT
Comment on attachment 57625 [details]
Patch

Clearing flags on attachment: 57625

Committed r60556: <http://trac.webkit.org/changeset/60556>
Comment 12 WebKit Commit Bot 2010-06-02 03:46:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Alexey Proskuryakov 2010-06-02 13:08:00 PDT
Mitz points out that we shouldn't modify tests in /css1. Can you roll out this patch?

> Ok.  I thought of a more witty retort...

So suppose HTML5 changes in some way, and html5lib tests change consequently. Practically speaking, how will we ensure that upgrading our copy won't degrade coverage in areas that this patch delegated to html5lib tests?

Delegating test coverage to a highly volatile external suite doesn't sound like a good idea to me, because upgrading responsibly will be a pain. Do we know if any policy governs maintenance of the suite? I'd expect that its goal is to test HTM5 requirements as well as possible, which is close to our goals, but doesn't align with them 100%.
Comment 14 Adam Barth 2010-06-02 13:21:29 PDT
(In reply to comment #13)
> Mitz points out that we shouldn't modify tests in /css1. Can you roll out this patch?

I think Eric agreed to undo the changes to /css1 via email.  I'm not sure who was CCed on the thread, but I'd expect those changes shortly.

> > Ok.  I thought of a more witty retort...
> 
> So suppose HTML5 changes in some way, and html5lib tests change consequently. Practically speaking, how will we ensure that upgrading our copy won't degrade coverage in areas that this patch delegated to html5lib tests?
>
> Delegating test coverage to a highly volatile external suite doesn't sound like a good idea to me, because upgrading responsibly will be a pain. Do we know if any policy governs maintenance of the suite? I'd expect that its goal is to test HTM5 requirements as well as possible, which is close to our goals, but doesn't align with them 100%.

Alexey, I'm not looking to argue with you, but I'm not sure you have all the facts right.  My understanding is that there are two parser behaviors at issue here:

1) Unterminated <title> tags.  This is covered both by the html5lib test suite and by:

http://trac.webkit.org/browser/trunk/LayoutTests/fast/tokenizer/missing-title-end-tag-1.html
http://trac.webkit.org/browser/trunk/LayoutTests/fast/tokenizer/missing-title-end-tag-2.html

as mentioned in Comment #9.

2) Nested start tags (e.g., "<a<b").  This is covered both by the html5lib test suite and by 

http://trac.webkit.org/browser/trunk/LayoutTests/fast/invalid/016.html

as mentioned in the ChangeLog.

Is there something else specific you're worried about?  I read, understood, and acted on your feedback.  You've already said it was the same as my review in comment #2 of the original patch.  I'm not sure why we're still arguing about this.
Comment 15 Eric Seidel (no email) 2010-06-02 13:45:57 PDT
Committed r60576: <http://trac.webkit.org/changeset/60576>
Comment 16 Alexey Proskuryakov 2010-06-02 14:44:17 PDT
Looks like I did have my facts wrong.

My feeling was that each unclosed tag's handling was (or could potentially be) different, and only some cases were covered. Looking at the ChangeLog again, I see that I misunderstood its structure - there are two lists in it, not one.