tables/mozilla/bugs/bug53690-1.html has never worked Requested by abarth on #webkit.
Created attachment 106705 [details] Patch
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.
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. :)
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.
(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. :)
> 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. :)
> 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.
(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.
> 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.
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?
Comment on attachment 106705 [details] Patch Sure. Thanks.
Created attachment 110980 [details] Patch
Comment on attachment 110980 [details] Patch SGTM.
Comment on attachment 110980 [details] Patch Clearing flags on attachment: 110980 Committed r97459: <http://trac.webkit.org/changeset/97459>
All reviewed patches have been landed. Closing bug.