Bug 39985 – Several layout tests needlessly use invalid HTML

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Several layout tests needlessly use invalid HTML

[Eric Seidel (no email)]

Created attachment 57525 [details]
Patch

[Adam Barth]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

@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.

[Alexey Proskuryakov]

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.

[Adam Barth]

Ah, thanks.  I misunderstood your comment.

[Eric Seidel (no email)]

Created attachment 57625 [details]
Patch

[Alexey Proskuryakov]

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?

[Adam Barth]

> 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

[Adam Barth]

> > 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.

[WebKit Commit Bot]

Comment on attachment 57625 [details]
Patch

Clearing flags on attachment: 57625

Committed r60556: <http://trac.webkit.org/changeset/60556>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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%.

[Adam Barth]

(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.

[Eric Seidel (no email)]

Committed r60576: <http://trac.webkit.org/changeset/60576>

[Alexey Proskuryakov]

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.