Bug 109173 – [EFL] Mark some tests are passing with incorrect expectations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[KwangYong Choi]

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

[KwangYong Choi]

Created attachment 187053 [details]
Patch

[Build Bot]

Comment on attachment 187053 [details]
Patch

Attachment 187053 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16429210

[Gyuyoung Kim]

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] ?

[KwangYong Choi]

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.

[Gyuyoung Kim]

(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.

[Chris Dumez]

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 :/

[KwangYong Choi]

(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?

[Chris Dumez]

(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.

[KwangYong Choi]

Created attachment 187252 [details]
Patch

Some tests in fast/forms/ are marked as passing tests with incorrect expectations.

[Chris Dumez]

Comment on attachment 187252 [details]
Patch

LGTM. Thanks.

[Chris Dumez]

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

[Gyuyoung Kim]

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.

[KwangYong Choi]

Created attachment 187258 [details]
Patch

Apply Christophe and Gyuyoung has mentioned.

[Chris Dumez]

Looks good. cq=me

[WebKit Review Bot]

Comment on attachment 187258 [details]
Patch

Clearing flags on attachment: 187258

Committed r142244: <http://trac.webkit.org/changeset/142244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.