Bug 88870 – [Qt]New API tests introuduced in r119723 marked as fail, but pass

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

See https://bugs.webkit.org/show_bug.cgi?id=88531#c4 for details.

[Csaba Osztrogonác]

Created attachment 148342 [details]
Patch

It is a speculative fix, because I don't know _anything_ about QML testing ... but now it passes. Am I correct?

[Csaba Osztrogonác]

ping?

[Andras Becsi]

Comment on attachment 148342 [details]
Patch

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

This is a bit confusing. Was the testcase marked as expected fail but passed already when it was checked in? Or was there a recent change which made the test work?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml:40
> +            expectFail("", "This currently fails since window.devicePixelRatio doesn't return the updated value.")

If the test passes, I think we do not have to re-add this line here.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml:58
> +            expectFail("", "This currently fails since the devicePixelRatio from the QML API isn't propagated correctly.")

Ditto.

[Csaba Osztrogonác]

It reported two XPASS (as I mentioned in https://bugs.webkit.org/show_bug.cgi?id=88531#c4):

XPASS  : qmltests::DevicePixelRatio::test_devicePixelRatio() '' returned TRUE unexpectedly. ()
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml(33)]
XPASS  : qmltests::DevicePixelRatio::test_devicePixelRatioMediaQuery() '' returned TRUE unexpectedly. ()
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml(51)]


The first check marked as expected to fail, but pass. If I removed
these expected fails, there are two fails later - where I moved
expected fail to. As I said, it is speculative fix, because I don't
know _anything_ about QML and devicePixelRatio ... It would be great
if the author of the original patch can check it.

[Csaba Osztrogonác]

Alex, Kenneth, any opinion? API tests haven't been pass 100% since 
a month because of this bug. Please comment this bug at least if 
you don't want (or don't have time) to fix the test you committed.

[Andras Becsi]

(In reply to comment #4)
> It reported two XPASS (as I mentioned in https://bugs.webkit.org/show_bug.cgi?id=88531#c4):
> 
> XPASS  : qmltests::DevicePixelRatio::test_devicePixelRatio() '' returned TRUE unexpectedly. ()
>    Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml(33)]
> XPASS  : qmltests::DevicePixelRatio::test_devicePixelRatioMediaQuery() '' returned TRUE unexpectedly. ()
>    Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml(51)]
> 
> 
> The first check marked as expected to fail, but pass. If I removed
> these expected fails, there are two fails later - where I moved
> expected fail to. As I said, it is speculative fix, because I don't
> know _anything_ about QML and devicePixelRatio ... It would be great
> if the author of the original patch can check it.

Although I'm not familiar with the QML testing infrastructure, this looks pretty much like the expectFail calls are not in the right position which makes your patch correct IMHO.

If someone sheds more light on, and eventually confirms this I would suggest to rename the bug to something like
"[Qt]New API tests introuduced in r119723 should correctly be marked as expected fail"

[Csaba Osztrogonác]

Alex, Kenneth, please answer and don't ignore this bug!

The API tests haven't been pass because of your buggy patch for 1.5 months.
Have you got any idea why they tests fail and could you fix them?

(( Alexis: That's why API tests will never make bots red ... because folks 
don't care about API tests at all ... We don't need always red bot. ))

[Kenneth Rohde Christiansen]

Comment on attachment 148342 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml:40
>> +            expectFail("", "This currently fails since window.devicePixelRatio doesn't return the updated value.")
> 
> If the test passes, I think we do not have to re-add this line here.

Agree; do you know what patch fixed this test?

[Csaba Osztrogonác]

(In reply to comment #8)
> (From update of attachment 148342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148342&action=review
> 
> >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_devicePixelRatio.qml:40
> >> +            expectFail("", "This currently fails since window.devicePixelRatio doesn't return the updated value.")
> > 
> > If the test passes, I think we do not have to re-add this line here.
> 
> Agree; do you know what patch fixed this test?

It passes from the beginning ...

[Csaba Osztrogonác]

PING again and again ...

I really don't undestand why do we have unittests if someone can commit 
failing API tests and then ignore answering why they are failing for months ...

I suggest we should simply revert the original patch if you aren't interested
in fixing it and when you simply ignore this bug report ...

[Csaba Osztrogonác]

Created attachment 157011 [details]
Patch

revert the original patch

[Alexis Menard (darktears)]

Comment on attachment 157011 [details]
Patch

Broken tests are useless. Please re-land with the correct expectation.

[Csaba Osztrogonác]

Comment on attachment 157011 [details]
Patch

Clearing flags on attachment: 157011

Committed r124923: <http://trac.webkit.org/changeset/124923>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.