Closed Bug 546635 Opened 11 years ago Closed 11 years ago

[HTML5] Update html5lib test suite snapshot

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: jgriffin)

References

Details

Attachments

(1 file, 1 obsolete file)

patch v2
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.
I'll take care of this.
Assignee: nobody → jgriffin
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?
Attached patch Update html5lib tests (obsolete) — — Splinter Review 
I've also modified the tests to turn on html5.enable, otherwise the tests will fail.
Status: NEW → ASSIGNED
(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.
(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?
(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.
Attached patch patch v2 — — Splinter Review 
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)
Comment on attachment 430771 [details] [diff] [review]
patch v2

Looks good.
Attachment #430771 - Flags: review?(hsivonen) → review+
Duplicate of this bug: 482916
Pushed as http://hg.mozilla.org/mozilla-central/rev/6f90a7b2bc13
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.