Bug 67745 – tables/mozilla/bugs/bug53690-1.html has never worked

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[WebKit Review Bot]

tables/mozilla/bugs/bug53690-1.html has never worked
Requested by abarth on #webkit.

[Adam Barth]

Created attachment 106705 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 106705 [details]
Patch

Yay!

I am not sure it’s a good idea to remove this test. It is not easy to see how it is redundant.

[Eric Seidel (no email)]

The test appears broken, all it was doing is printing this:
CONSOLE MESSAGE: line 4: Uncaught TypeError: Cannot read property 'action' of undefined

I don't think the test's intention was to document that document.forms[1] returns unsigned when there is only one form on the page. :)

[Eric Seidel (no email)]

The original bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=53690

Original test:
https://bug53690.bugzilla.mozilla.org/attachment.cgi?id=15971

The bug seems to be about table reflow.

It's possible the test should just be modified to remove the action= part of the change, since all it seems to want to test is the document.write behavior.

The test was noticed because Chromium "fails" the test due to differnet v8 messages (a long trail of tears).  Adam removed it, seeking to remove always-broken tests.

(In reply to comment #4)
> The test appears broken, all it was doing is printing this:
> CONSOLE MESSAGE: line 4: Uncaught TypeError: Cannot read property 'action' of undefined

Looking at platform/mac/tables/mozilla/bugs, I see that the test was producing a render tree dump and a pixel result.

> I don't think the test's intention was to document that document.forms[1] returns unsigned when there is only one form on the page. :)

Agreed.

(In reply to comment #5)
 > The test was noticed because Chromium "fails" the test due to differnet v8 messages (a long trail of tears).

Seems like a great test! We should have more test that uncover quirks and other problems, not fewer.

[Eric Seidel (no email)]

(In reply to comment #6)
> (In reply to comment #5)
>  > The test was noticed because Chromium "fails" the test due to differnet v8 messages (a long trail of tears).
> 
> Seems like a great test! We should have more test that uncover quirks and other problems, not fewer.

This is a well documented "quirk". :)  If that were the only thing this test provided, it would not be a very good test for such. :)

[Adam Barth]

> Seems like a great test! We should have more test that uncover quirks and other problems, not fewer.

Fortunately (or unfortunately) we have lots of test coverage for that particular issue.  :)

[Adam Barth]

> It's possible the test should just be modified to remove the action= part of the change, since all it seems to want to test is the document.write behavior.

Unfortunately, the document.write was never happening because the JavaScript threw an exception before it executed the write.

We can change the test to do something else, but this test has never been testing anything, as far as I can tell.

(In reply to comment #9)
> this test has never been testing anything, as far as I can tell.

I have a render tree dump and an image saying otherwise.

[Adam Barth]

(In reply to comment #10)
> (In reply to comment #9)
> > this test has never been testing anything, as far as I can tell.
> 
> I have a render tree dump and an image saying otherwise.

Can you explain what this test is testing?

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > this test has never been testing anything, as far as I can tell.
> > 
> > I have a render tree dump and an image saying otherwise.
> 
> Can you explain what this test is testing?

It is testing (primarily table-) layout and rendering code.

[Adam Barth]

> It is testing (primarily table-) layout and rendering code.

Ok.  If you're not interested in actually engaging in this discussion, we can leave this test in its useless state.

(In reply to comment #13)
> > It is testing (primarily table-) layout and rendering code.
> 
> Ok.  If you're not interested in actually engaging in this discussion, we can leave this test in its useless state.

While I’ll be happy with that result (leaving the test in its current state, which is not useless), I am sorry that you think that I am not interested in discussing this. Let me try again: detecting changes in behavior, which are possibly bugs, is one of the main reasons we have regression tests. Running the tests after making a code change often reveals an unanticipated, undesirable effect of the change on one ore more tests. It is not easy to determine that any unique test is redundant, in the sense that removing it will not allow an undesirable code change to sneak in; so in principle, the argument for removing a test should be more compelling than that it is not immediately obvious how a code change could cause it to fail without affecting any one of the remaining tests.

[Adam Barth]

I assume you'll agree that this test is not intended to test HTML parsing.  We have lots of test coverage for how the parser works in these situations.  That means the important aspect is the DOM, which is the following:

html
  body
    table
      tbody
        tr
          td
            table border=1
              tbody
                tr
                  td
                    script
          td
            "x"

which is almost identical to tables/mozilla/bugs/bug53690-2.html and a number of other table tests.

The unique thing that this test was trying test was the interaction of document.writing a form in the middle of this process.  However, because that was never happening, this test wasn't actually testing anything different than many other tests.

Can this be resolved by removing the content of the script element in this test?

[Adam Barth]

Comment on attachment 106705 [details]
Patch

Sure.  Thanks.

[Adam Barth]

Created attachment 110980 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 110980 [details]
Patch

SGTM.

[WebKit Review Bot]

Comment on attachment 110980 [details]
Patch

Clearing flags on attachment: 110980

Committed r97459: <http://trac.webkit.org/changeset/97459>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.