Bug 88870

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?

ping?

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.

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.

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.

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

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

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?

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

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

Created attachment 157011 [details] Patch revert the original patch

Comment on attachment 157011 [details] Patch Broken tests are useless. Please re-land with the correct expectation.

Comment on attachment 157011 [details] Patch Clearing flags on attachment: 157011 Committed r124923: <http://trac.webkit.org/changeset/124923>

All reviewed patches have been landed. Closing bug.