Bug 105407

Summary: Current error reporting method used by check-layout.js should not affect subsequent sub-tests using checking data-offset-y.
Product: WebKit Reporter: Pravin D <pravind.2k4>
Component: Tools / TestsAssignee: Pravin D <pravind.2k4>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, jchaffraix, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
TestCase
none
Proped Patch
none
Patch none

Description Pravin D 2012-12-19 02:26:43 PST 
Description:
Consider a testcase which contains a set of sub-tests all checking if the element has correct data-offset-y(offsetTop).

When one of the sub-tests fails, the failure msg is appended to the elements container(in our case, its the sub-test). 
This extra content changes the offsetTop of other subsequent sub-tests and all sub-tests following the failing test case also fail.

This becomes confusing/time consuming when either creating a test-case or debugging test failure.
Comment 1 Pravin D 2012-12-19 07:50:51 PST 
Created attachment 180166 [details]
TestCase
Comment 2 Ojan Vafai 2012-12-19 09:40:44 PST 
Maybe we should save error messages + their containers into an array and only write them out at the end of the test? If we wanted to be paranoid, we could do it in a try/finally to ensure that we write out the errors even if the test throws an exception, although, it really shouldn't since all the relevant code is in check-layout.js.
Comment 3 Tony Chang 2012-12-19 11:00:19 PST 
(In reply to comment #2)
> Maybe we should save error messages + their containers into an array and only write them out at the end of the test? If we wanted to be paranoid, we could do it in a try/finally to ensure that we write out the errors even if the test throws an exception, although, it really shouldn't since all the relevant code is in check-layout.js.

Some other options:

We currently append PASS/FAIL next to the parent node, or if the parent node has the container class, we append next to the parent's parent.  That's kind of hacky.  We could add an optional param to checkLayout() for the node to append results to (and make the PASS text more clear like listing the id or class of the passing node).

Alternately, we could add an attribute like data-log-results="some-div-id". If we see that, we write the results to some-div-id.

Anyway, I think any of these approaches are fine.  It's a little weird that for some of the existing writing mode tests the word "PASS" ends up being written vertically.
Comment 4 Ojan Vafai 2012-12-19 13:15:44 PST 
(In reply to comment #3)
> We currently append PASS/FAIL next to the parent node, or if the parent node has the container class, we append next to the parent's parent.  That's kind of hacky.  We could add an optional param to checkLayout() for the node to append results to (and make the PASS text more clear like listing the id or class of the passing node).

I agree this is less hacky, but it's also harder to use with tests that have lots of test-cases. It's hard to match up the failure to the test case.

> Alternately, we could add an attribute like data-log-results="some-div-id". If we see that, we write the results to some-div-id.

Same issue, right? Also, I don't like adding more work for people to do when writing these tests.

> Anyway, I think any of these approaches are fine.  It's a little weird that for some of the existing writing mode tests the word "PASS" ends up being written vertically.

I agree that it's unfortunate that the logging output affects the tests and is affected by the tests. We could use shadow-dom with resetStyleInheritance. :)

More seriously, with my original proposal, we could put them in a separate container div at the end and absolutely position the elements after the test-case to minimize the affects between tests and their output?

I'd really like to keep the behavior of having the failure output be next to the test case if possible.
Comment 5 Tony Chang 2012-12-19 13:33:10 PST 
All my suggestions were for an optional parameter.  The default behavior would be to do what we currently do.  I don't think there are that many tests that need to set a custom output location.  We shouldn't need to edit or rebaseline any existing tests.

I think it would be hard to know where to place the result using absolute positioning.

The other option would be to do nothing.  In the attached test case, you can easily avoid the problem by doing checkLayout('body'), although you only get a single PASS or FAIL.  If you want a separate PASS/FAIL for each test case, you could wrap each test in a position:relative div.  We do that for a lot of the flexbox tests.
Comment 6 Pravin D 2012-12-19 15:03:22 PST 
Created attachment 180234 [details]
Proped Patch
Comment 7 Pravin D 2012-12-19 15:05:58 PST 
Comment on attachment 180234 [details]
Proped Patch

Reversing the order in which nodes are processed should ideally fix the issue. 
@Ojan, @Tony please let me know ur opinions.
Also am not sure where to add a test case for this fix... Suggestion req on the same.
Comment 8 WebKit Review Bot 2012-12-19 15:55:15 PST 
Comment on attachment 180234 [details]
Proped Patch

Attachment 180234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15401804

New failing tests:
css3/flexbox/flex-algorithm-with-margins.html
css3/flexbox/box-sizing.html
css3/flexbox/flex-flow-margins.html
css3/flexbox/flex-align.html
animations/suspend-resume-animation-events.html
css3/flexbox/flex-align-percent-height.html
css3/flexbox/flex-align-vertical-writing-mode.html
css3/flexbox/align-absolute-child.html
css3/flexbox/flex-item-min-size.html
css3/flexbox/flex-no-flex.html
css3/flexbox/flex-algorithm.html
css3/flexbox/columns-height-set-via-top-bottom.html
css3/flexbox/flex-flow.html
css3/flexbox/flex-align-column.html
css3/flexbox/flex-algorithm-min-max.html
css3/flexbox/flex-item-child-overflow.html
css3/flexbox/flex-flow-margins-auto-size.html
css3/flexbox/flex-align-max.html
css3/flexbox/auto-height-dynamic.html
css3/flexbox/flex-flow-auto-margins.html
css3/flexbox/flex-flow-padding.html
css3/flexbox/columns-auto-size.html
css3/flexbox/flex-align-end.html
css3/flexbox/flex-flow-orientations.html
css3/flexbox/flex-flow-overflow.html
css3/flexbox/flex-align-stretch.html
css3/flexbox/flex-flow-auto-margins-no-available-space.html
css3/flexbox/flex-flow-border.html
css3/flexbox/box-sizing-min-max-sizes.html
css3/flexbox/flex-justify-content.html
Comment 9 Build Bot 2012-12-19 17:38:47 PST 
Comment on attachment 180234 [details]
Proped Patch

Attachment 180234 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15403871

New failing tests:
css3/flexbox/flex-algorithm-with-margins.html
css3/flexbox/box-sizing.html
css3/flexbox/flex-flow-margins.html
css3/flexbox/flex-align.html
css3/flexbox/flex-align-percent-height.html
css3/flexbox/flex-align-vertical-writing-mode.html
css3/flexbox/align-absolute-child.html
css3/flexbox/flex-item-min-size.html
css3/flexbox/flex-no-flex.html
css3/flexbox/flex-algorithm.html
css3/flexbox/columns-height-set-via-top-bottom.html
css3/flexbox/flex-flow.html
css3/flexbox/flex-align-column.html
css3/flexbox/flex-algorithm-min-max.html
css3/flexbox/flex-item-child-overflow.html
css3/flexbox/flex-flow-margins-auto-size.html
css3/flexbox/flex-align-max.html
css3/flexbox/auto-height-dynamic.html
css3/flexbox/flex-flow-auto-margins.html
css3/flexbox/flex-flow-padding.html
css3/flexbox/columns-auto-size.html
css3/flexbox/flex-align-end.html
css3/flexbox/flex-flow-orientations.html
css3/flexbox/flex-flow-overflow.html
compositing/overflow/overflow-auto-with-touch-toggle.html
css3/flexbox/flex-align-stretch.html
css3/flexbox/flex-flow-auto-margins-no-available-space.html
css3/flexbox/flex-flow-border.html
css3/flexbox/box-sizing-min-max-sizes.html
css3/flexbox/flex-justify-content.html
Comment 10 Tony Chang 2012-12-20 10:35:30 PST 
Comment on attachment 180234 [details]
Proped Patch

CONSOLE MESSAGE: line 163: Uncaught TypeError: [object Array] is not a function is what the tests are spewing.
Comment 11 Pravin D 2012-12-20 12:56:49 PST 
Created attachment 180392 [details]
Patch
Comment 12 Pravin D 2012-12-20 12:58:29 PST 
(In reply to comment #10)
> (From update of attachment 180234 [details])
> CONSOLE MESSAGE: line 163: Uncaught TypeError: [object Array] is not a function is what the tests are spewing.
> 
I extremely sorry for this. Have uploaded another patch, with which test case do not seem to fail.
Comment 13 Tony Chang 2012-12-20 13:42:31 PST 
Comment on attachment 180392 [details]
Patch

Seems fine to me. Ojan might have an opinion, but he won't be back from vacation until January.
Comment 14 Ojan Vafai 2013-01-29 12:25:40 PST 
Comment on attachment 180392 [details]
Patch

I'm a bit torn on whether this is the best solution. I still somewhat prefer the approach of saving everything in an error array and then writing out all the errors at the end. But, I don't see any immediate problems with this simpler approach.
Comment 15 WebKit Review Bot 2013-01-29 12:55:48 PST 
Comment on attachment 180392 [details]
Patch

Clearing flags on attachment: 180392

Committed r141147: <http://trac.webkit.org/changeset/141147>
Comment 16 WebKit Review Bot 2013-01-29 12:55:52 PST 
All reviewed patches have been landed.  Closing bug.