Closed
Bug 546635
Opened 15 years ago
Closed 15 years ago
[HTML5] Update html5lib test suite snapshot
Categories
(Core :: DOM: HTML Parser, defect, P1)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
People
(Reporter: hsivonen, Assigned: jgriffin)
References
Details
Attachments
(1 file, 1 obsolete file)
118.45 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
parser/htmlparser/tests/mochitest/test_html5_tree_construction.html fails with the HTML5 parser, because the snapshot of the test suite isn't up-to-date.
Assignee | ||
Comment 2•15 years ago
|
||
Henri, I've updated the files that come from html5lib, but there is another file, regressions.txt, that is causing 7 failures:
Unexpected Failure:
Matched: false
Input: <body><body><base><link><meta><title><p></title><body><p>#</body>
Expected:
|<html>
<head>
<base>
<meta>
<title>
"<p>"
<body>
<link>
<p>
"#"|
-
Output:
|<html>
<head>
<body>
<base>
<link>
<meta>
<title>
"<p>"
<p>
"#"|
Unexpected Failure:
Matched: false
Input: <!doctype html><body><title>X</title><meta name=y><link rel=foo><style> x { content:"</style" } </style>
Expected:
|<!DOCTYPE html>
<html>
<head>
<title>
"X"
<meta>
name="y"
<body>
<link>
rel="foo"
<style>
"
x { content:"</style" } "|
-
Output:
|<!DOCTYPE html>
<html>
<head>
<body>
<title>
"X"
<meta>
name="y"
<link>
rel="foo"
<style>
"
x { content:"</style" } "|
Unexpected Failure:
Matched: false
Input: <!DOCTYPE html><body><table><tr><td></td><input tYPe=" hiDDen "><td></td></tr></table></body>
Expected:
|<!DOCTYPE html>
<html>
<head>
<body>
<table>
<tbody>
<tr>
<td>
<input>
type="hiDDen"
<td>|
-
Output:
|<!DOCTYPE html>
<html>
<head>
<body>
<input>
type=" hiDDen "
<table>
<tbody>
<tr>
<td>
<td>|
Unexpected Failure:
Matched: false
Input: <!DOCTYPE html><html><head></head><form><input></form><frameset rows="*"><frame></frameset></html>
Expected:
|<!DOCTYPE html>
<html>
<head>
<frameset>
rows="*"
<frame>|
-
Output:
|<!DOCTYPE html>
<html>
<head>
<body>
<form>
<input>|
Unexpected Failure:
Matched: false
Input: <!DOCTYPE html><html><head></head><form><input type="hidden"></form><frameset rows="*"><frame></frameset></html>
Expected:
|<!DOCTYPE html>
<html>
<head>
<frameset>
rows="*"
<frame>|
-
Output:
|<!DOCTYPE html>
<html>
<head>
<body>
<form>
<input>
type="hidden"|
Unexpected Failure:
Matched: false
Input: <!DOCTYPE html><html><head></head><form><input tYpE=" HIdDen "></form><frameset rows="*"><frame></frameset></html>
Expected:
|<!DOCTYPE html>
<html>
<head>
<frameset>
rows="*"
<frame>|
-
Output:
|<!DOCTYPE html>
<html>
<head>
<body>
<form>
<input>
type=" HIdDen "|
Unexpected Failure:
Matched: false
Input: <!DOCTYPE html><html><body><table><link><tr><td>Hi!</td></tr></table></html>
Expected:
|<!DOCTYPE html>
<html>
<head>
<body>
<table>
<link>
<tbody>
<tr>
<td>
"Hi!"|
-
Output:
|<!DOCTYPE html>
<html>
<head>
<body>
<link>
<table>
<tbody>
<tr>
<td>
"Hi!"|
I think these are probably all OK and should be removed, or possibly we should remove regressions.txt altogether. What's your opinion?
Assignee | ||
Comment 3•15 years ago
|
||
I've also modified the tests to turn on html5.enable, otherwise the tests will fail.
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•15 years ago
|
||
(In reply to comment #2)
> Henri, I've updated the files that come from html5lib, but there is another
> file, regressions.txt, that is causing 7 failures:
>
> Unexpected Failure:
>
> Matched: false
>
> Input: <body><body><base><link><meta><title><p></title><body><p>#</body>
>
> Expected:
> |<html>
> <head>
> <base>
> <meta>
> <title>
> "<p>"
> <body>
> <link>
> <p>
> "#"|
How did you get the expected form? Do you have an up-to-date copy of the JS version of the HTML5 parser?
> Input: <!DOCTYPE html><body><table><tr><td></td><input tYPe=" hiDDen
> "><td></td></tr></table></body>
>
> Expected:
> |<!DOCTYPE html>
> <html>
> <head>
> <body>
> <table>
> <tbody>
> <tr>
> <td>
> <input>
> type="hiDDen"
> <td>|
It seems to me the expectation is wrong.
> I think these are probably all OK and should be removed, or possibly we should
> remove regressions.txt altogether. What's your opinion?
I think it would be good to keep a mechanism for flagging some test as "expected not to pass".
(In reply to comment #3)
> Created an attachment (id=427641) [details]
> Update html5lib tests
>
> I've also modified the tests to turn on html5.enable, otherwise the tests will
> fail.
Does this work without making subsequent tests orange when the old parser in on by default? That kind of thing has been a problem in the past. It might be safer to skip the test if html5.enable=false.
Assignee | ||
Comment 5•15 years ago
|
||
(In reply to comment #4)
The expected versions come directly from regressions.txt. AFAICT, this file was generated years ago when the then-current parser caused regressions that were subsequently fixed. Since the parser rules have changed significantly since then, these are really out-of-date. We need either to update the expected versions based on HTML5, or just remove them altogether. Since the original codebase which was involved in the regressions is not active in the HTML5 parser (I think?), I believe it only makes sense to update these (rather than remove them), if they are interesting cases in and of themselves that aren't already covered by the html5lib tests.
> I think it would be good to keep a mechanism for flagging some test as
> "expected not to pass".
We have such a mechanism in exceptions.js, which is distinct from regressions.txt.
> Does this work without making subsequent tests orange when the old parser in
> on by default? That kind of thing has been a problem in the past. It might be
> safer to skip the test if html5.enable=false.
This turns the HTML5 parser on for these tests, regardless of which parser is on by default. So I don't think it should cause any problems in any configuration. The only way it would break is if the HTML 5 parser were not available, so that setting html5.enable=true had no effect. Are there cases where that could be true?
Reporter | ||
Comment 6•15 years ago
|
||
(In reply to comment #5)
> The expected versions come directly from regressions.txt. AFAICT, this file
> was generated years ago when the then-current parser caused regressions that
> were subsequently fixed. Since the parser rules have changed significantly
> since then, these are really out-of-date. We need either to update the
> expected versions based on HTML5, or just remove them altogether. Since the
> original codebase which was involved in the regressions is not active in the
> HTML5 parser (I think?), I believe it only makes sense to update these (rather
> than remove them), if they are interesting cases in and of themselves that
> aren't already covered by the html5lib tests.
Oh. If regressions.txt is related to the old nsHTMLContentSink-using parser, let's get rid of regressions.txt.
> > Does this work without making subsequent tests orange when the old parser in
> > on by default? That kind of thing has been a problem in the past. It might be
> > safer to skip the test if html5.enable=false.
>
> This turns the HTML5 parser on for these tests, regardless of which parser is
> on by default. So I don't think it should cause any problems in any
> configuration. The only way it would break is if the HTML 5 parser were not
> available, so that setting html5.enable=true had no effect. Are there cases
> where that could be true?
html5.enable=true is available. There's no switch to make it unavailable.
The situation I was concerned about is that previously, touching the html5.enable pref from a mochitest has leaked to subsequent tests because an innerHTML fragment parser got cached under html5.enable=true and the cached object was reused under html5.enable=false.
If this particular test doesn't cause orange in subsequent tests, I guess it's OK to flip the pref from within the test.
Assignee | ||
Comment 7•15 years ago
|
||
I removed regressions.txt, and ran the patch on the tryserver; no problems were found in subsequent tests by flipping the html5.enable=true pref on this test.
Attachment #427641 -
Attachment is obsolete: true
Attachment #430771 -
Flags: review?(hsivonen)
Reporter | ||
Comment 8•15 years ago
|
||
Comment on attachment 430771 [details] [diff] [review]
patch v2
Looks good.
Attachment #430771 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 10•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•