Bug 95613 – fast/table/empty-row-crash.html and fast/table/inline-form-assert.html should be dumpAsText tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Julien Chaffraix]

While working on a change, I stumbled upon those 2 tests which are testing a crash and an assert.

It shouldn't be difficult to convert them to dumpAsText and remove a lot of baselines at the same time.

[Pravin D]

Created attachment 161886 [details]
Patch

[Julien Chaffraix]

CC'ing the original authors and reviewers of the tests in case they have some objection.

[Julien Chaffraix]

Comment on attachment 161886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161886&action=review

> LayoutTests/ChangeLog:8
> +         Modified the test cases empty-row-crash.html and inline-form-assert.html to dumpAsText tests. 

It's always nice to explain the 'why' behind a change. Here it should outline why we don't lose coverage by doing this conversion.

> LayoutTests/ChangeLog:9
> +         Removed obsolete baselines.

You could put that below the removed baselines.

> LayoutTests/fast/table/empty-row-crash.html:15
> +In order to insert a row into a table atleast one column must exist. Otherwise it will result in a crash.
> +As an empty table will have a minimum of one column, inserting a row into such a table should not result in a crash.<br><br>

Please don't insert this description as I am not convinced it is a fine assumption to make. Always having a column means that a table without any cells takes some width due to border-spacing but it may not take any height (f.e. <table><tbody></tbody></table>). Browsers are not consistent on that and CSS 2.1 is not super clear on what we should do so I would rather not document our behavior as we may decide to change that.

Something without making those assumptions would do:

This test checks that inserting a row in an empty table doesn't crash.

> LayoutTests/fast/table/empty-row-crash.html:16
> +No crash means test PASS.

Shouldn't it be:

No crash means that the test PASSED ?

(I am not a native speaker but this sentence sounds better to me)

> LayoutTests/fast/table/inline-form-assert.html:14
> +    Test for <i><a href="http://webkit.org/b/12373">http://webkit.org/b/12373</a>

Nit: This is an unneeded change as both URL are equivalent. I don't mind the shorter version though.

[Pravin D]

Created attachment 162342 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162342 [details]
Patch

Clearing flags on attachment: 162342

Committed r127813: <http://trac.webkit.org/changeset/127813>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.