Bug 109173

Summary: [EFL] Mark some tests are passing with incorrect expectations
Product: WebKit Reporter: KwangYong Choi <ky0.choi>
Component: WebKit EFLAssignee: KwangYong Choi <ky0.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, laszlo.gombos, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
cdumez: commit-queue-
Patch none

Description KwangYong Choi 2013-02-07 04:04:12 PST
These expectations are added by r140250. But the tests are failing and they are marked as Missing on TestExpectations.

*  fast/forms/basic-textareas-quirks.html
*  fast/forms/input-disabled-color.html
*  fast/forms/input-readonly-dimmed.html
*  fast/forms/listbox-hit-test-zoomed.html
*  fast/forms/menulist-narrow-width.html
*  fast/forms/menulist-style-color.html
*  fast/forms/plaintext-mode-2.html
*  fast/forms/search-cancel-button-style-sharing.html
*  fast/forms/search-rtl.html
*  fast/forms/select-baseline.html
*  fast/forms/select-style.html
*  fast/forms/zoomed-controls.html
Comment 1 KwangYong Choi 2013-02-07 04:06:05 PST
Created attachment 187053 [details]
Patch
Comment 2 Build Bot 2013-02-07 09:53:53 PST
Comment on attachment 187053 [details]
Patch

Attachment 187053 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16429210
Comment 3 Gyuyoung Kim 2013-02-07 20:44:15 PST
Comment on attachment 187053 [details]
Patch

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

> LayoutTests/ChangeLog:7
> +        But the tests are failing and they are marked as Missing on TestExpectations.

Is it better to change the result of expectation as [Failure] ?
Comment 4 KwangYong Choi 2013-02-07 21:07:29 PST
Comment on attachment 187053 [details]
Patch

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

>> LayoutTests/ChangeLog:7
>> +        But the tests are failing and they are marked as Missing on TestExpectations.
> 
> Is it better to change the result of expectation as [Failure] ?

Actually, the tests are passing with wrong expectations.
Comment 5 Gyuyoung Kim 2013-02-07 21:12:43 PST
(In reply to comment #4)

> Actually, the tests are passing with wrong expectations.

If so, I think it is correct to fix wrong expectation and actual result.
Comment 6 Chris Dumez 2013-02-07 22:41:55 PST
I don't think removing the expected results is best here. How about updating TestExpectations file so that those tests have [ Pass ] expectation, with a comment such as: "Their generated expected result is incorrect".

This way, we could easily notice if the bugs gets fixed in the future as the tests will start failing (and will need rebaseline). If you remove the results and keep their expectation as [ Missing ], those tests will not be executed :/
Comment 7 KwangYong Choi 2013-02-07 23:37:24 PST
(In reply to comment #6)
> I don't think removing the expected results is best here. How about updating TestExpectations file so that those tests have [ Pass ] expectation, with a comment such as: "Their generated expected result is incorrect".
> 
> This way, we could easily notice if the bugs gets fixed in the future as the tests will start failing (and will need rebaseline). If you remove the results and keep their expectation as [ Missing ], those tests will not be executed :/

I understand what you mean and I remember that you have mentioned it before. But I'm not sure it's possible to update TestExpectations like below.

Local test is ok. We can get the results as intended. But WebKitTestRunner handles them *REAL* pass.

  21 # 4. SUCCESS TESTS WITH INCORRECT EXPECTATIONS
  22 #    * Test cases that are passing with their generated incorrect expectations.
  23 #    * It should be checked if the test fails. Remove the line if the test result is correct
  24 #      otherwise update with new incorrect one.
  25 

 841 #////////////////////////////////////////////////////////////////////////////////////////
 842 # SUCCESS TESTS WITH INCORRECT EXPECTATIONS
 843 #////////////////////////////////////////////////////////////////////////////////////////
 844 
 845 # Regression from r133898
 846 Bug(EFL) fast/forms/basic-textareas-quirks.html [ Pass ]

Any idea?
Comment 8 Chris Dumez 2013-02-07 23:45:02 PST
(In reply to comment #7)
> (In reply to comment #6)
> > I don't think removing the expected results is best here. How about updating TestExpectations file so that those tests have [ Pass ] expectation, with a comment such as: "Their generated expected result is incorrect".
> > 
> > This way, we could easily notice if the bugs gets fixed in the future as the tests will start failing (and will need rebaseline). If you remove the results and keep their expectation as [ Missing ], those tests will not be executed :/
> 
> I understand what you mean and I remember that you have mentioned it before. But I'm not sure it's possible to update TestExpectations like below.
> 
> Local test is ok. We can get the results as intended. But WebKitTestRunner handles them *REAL* pass.
> 
>   21 # 4. SUCCESS TESTS WITH INCORRECT EXPECTATIONS
>   22 #    * Test cases that are passing with their generated incorrect expectations.
>   23 #    * It should be checked if the test fails. Remove the line if the test result is correct
>   24 #      otherwise update with new incorrect one.
>   25 
> 
>  841 #////////////////////////////////////////////////////////////////////////////////////////
>  842 # SUCCESS TESTS WITH INCORRECT EXPECTATIONS
>  843 #////////////////////////////////////////////////////////////////////////////////////////
>  844 
>  845 # Regression from r133898
>  846 Bug(EFL) fast/forms/basic-textareas-quirks.html [ Pass ]
> 
> Any idea?

I think this works (except that I would add a comment that generated result is incorrect in TextExpectation for clarity). I'm not sure what you mean by *real* pass. Sure, WKTR will think the tests are passing but WebKit developers will know better thanks to TestExpectations.

The idea is that one day, those tests may start failing and then we will take a look and maybe notice that the new result is better. This way, we can more easily know which revision fixed the tests.
Comment 9 KwangYong Choi 2013-02-08 00:08:22 PST
Created attachment 187252 [details]
Patch

Some tests in fast/forms/ are marked as passing tests with incorrect expectations.
Comment 10 Chris Dumez 2013-02-08 00:24:59 PST
Comment on attachment 187252 [details]
Patch

LGTM. Thanks.
Comment 11 Chris Dumez 2013-02-08 00:26:25 PST
Comment on attachment 187252 [details]
Patch

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

Please fix these few types before we land this:

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=109173

"are" -> "as"

> LayoutTests/ChangeLog:6
> +        Unrevied EFL gardening.

Unreviewed

> LayoutTests/ChangeLog:8
> +        Add new cetegory for TestExpectations: PASSING TESTS WITH INCORRECT EXPECTATIONS.

category
Comment 12 Gyuyoung Kim 2013-02-08 00:33:11 PST
Comment on attachment 187252 [details]
Patch

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

Looks fine too.

> LayoutTests/platform/efl/TestExpectations:856
> +webkit.org/b/85877 fast/forms/menulist-style-color.html [ Pass ]

Wrong alphabet order.
Comment 13 KwangYong Choi 2013-02-08 00:39:51 PST
Created attachment 187258 [details]
Patch

Apply Christophe and Gyuyoung has mentioned.
Comment 14 Chris Dumez 2013-02-08 00:41:29 PST
Looks good. cq=me
Comment 15 WebKit Review Bot 2013-02-08 00:58:47 PST
Comment on attachment 187258 [details]
Patch

Clearing flags on attachment: 187258

Committed r142244: <http://trac.webkit.org/changeset/142244>
Comment 16 WebKit Review Bot 2013-02-08 00:58:52 PST
All reviewed patches have been landed.  Closing bug.