RESOLVED WONTFIX 61304
[Qt] QML WebView API tests need improvements
https://bugs.webkit.org/show_bug.cgi?id=61304
Summary [Qt] QML WebView API tests need improvements
Venkat Penukonda
Reported 2011-05-23 13:50:59 PDT
The tests in qdeclarativewebview.cpp needs improvements (at path Source/WebKit/qt/tests/qdeclarativewebview/ ). Currently the basicproperties() test case tries to test too many properties in one single test case. They need to be split into simpler test cases to test each property, so that when one test case fails, it is easy to narrow down which property failed the test. Same thing with historyNav() test case.
Attachments
Proposed patch (14.40 KB, patch)
2011-05-23 14:09 PDT, Venkat Penukonda
no flags
Modified patch (22.27 KB, patch)
2011-05-24 15:06 PDT, Venkat Penukonda
hausmann: review-
hausmann: commit-queue-
Updated patch (22.53 KB, patch)
2011-05-26 12:23 PDT, Venkat Penukonda
no flags
Patch updated and resubmitted incorporating the corrections suggested by the reviewer above. (23.24 KB, patch)
2011-05-27 11:59 PDT, Venkat Penukonda
no flags
Updated Patch (23.21 KB, patch)
2011-06-06 11:42 PDT, Venkat Penukonda
hausmann: review-
hausmann: commit-queue-
Venkat Penukonda
Comment 1 2011-05-23 14:09:45 PDT
Created attachment 94486 [details] Proposed patch
Alexis Menard (darktears)
Comment 2 2011-05-23 14:46:10 PDT
Comment on attachment 94486 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94486&action=review > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:568 > + checkNoErrors(component); This pattern is repetitive? Could you refactor that and put the component and engine common for all (and even perhaps all tests)?
Venkat Penukonda
Comment 3 2011-05-24 15:06:11 PDT
Created attachment 94697 [details] Modified patch
Venkat Penukonda
Comment 4 2011-05-24 15:07:53 PDT
Updated the patch as per above suggestion from Alexis.
Simon Hausmann
Comment 5 2011-05-25 17:37:43 PDT
Comment on attachment 94697 [details] Modified patch View in context: https://bugs.webkit.org/attachment.cgi?id=94697&action=review I think the general direction is correct, but I'm saying r- because of the nitpicks and because the patch should add only one entry to the ChangeLog, not two. > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:29 > +protected: > + QDeclarativeEngine* m_engine; > + QDeclarativeComponent* m_component; > + I suppose those should be private instead of protected? > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:75 > tst_QDeclarativeWebView::tst_QDeclarativeWebView() > { > - Q_UNUSED(waitForSignal) > + m_engine = new QDeclarativeEngine(); > + m_component= new QDeclarativeComponent(m_engine); > + Q_UNUSED(waitForSignal) If you want to minimize the risk of side-effects between the test cases, then you could create and destroy the engine/component in init() and cleanup(). > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:134 > + QCOMPARE(wv->property("title").toString(), QLatin1String("")); // empty string by default I suggest to use QVERIFY and QString::isEmpty() instead of comparing with an empty QLatin1String. > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:551 > +void tst_QDeclarativeWebView::settings_defaultFontSize() > +{ > + m_component->loadUrl(QUrl("qrc:///resources/basic.qml")); > + QObject* wv = m_component->create(); > + QVERIFY(wv); > + QTRY_COMPARE(wv->property("progress").toDouble(), 1.0); > + > + QObject* s = QDeclarativeProperty(wv, "settings").object(); > + QVERIFY(s); > + const QString& name = QString::fromAscii("defaultFontSize"); > + int value = QWebSettings::DefaultFontSize; > + s->setProperty(name.toAscii().data(), value); > + QCOMPARE(s->property(name.toAscii().data()).toInt(), value); > +} > +void tst_QDeclarativeWebView::settings_standardFontFamily() > +{ > + m_component->loadUrl(QUrl("qrc:///resources/basic.qml")); > + QObject* wv = m_component->create(); > + QVERIFY(wv); > + QTRY_COMPARE(wv->property("progress").toDouble(), 1.0); > + > + QObject* s = QDeclarativeProperty(wv, "settings").object(); > + QVERIFY(s); > + const QString& name = QString::fromAscii("standardFontFamily"); > + const QString& value = "Arial"; > + s->setProperty(name.toAscii().data(), value); > + QCOMPARE(s->property(name.toAscii().data()).toString(), value); > +} > + Why not use QTestLib's data driven testing for testing the symmetry of all the properties instead of duplicating all the code? Then you get the best of both: No duplicated code but if one property test fails the remaining tests are still executed.
Venkat Penukonda
Comment 6 2011-05-26 12:23:54 PDT
Created attachment 95019 [details] Updated patch Simon's suggestions above are incorporated into this updated patch.
Alexis Menard (darktears)
Comment 7 2011-05-27 05:04:10 PDT
Comment on attachment 95019 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=95019&action=review > ChangeLog:7 > + You need to explain what you are doing. > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:77 > + Q_UNUSED(waitForSignal) Indentation is wrong.
Venkat Penukonda
Comment 8 2011-05-27 11:59:39 PDT
Created attachment 95203 [details] Patch updated and resubmitted incorporating the corrections suggested by the reviewer above.
Alexis Menard (darktears)
Comment 9 2011-06-06 09:57:21 PDT
Comment on attachment 95203 [details] Patch updated and resubmitted incorporating the corrections suggested by the reviewer above. View in context: https://bugs.webkit.org/attachment.cgi?id=95203&action=review > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:113 > + m_component= new QDeclarativeComponent(m_engine); Indentation issue. 4 blank spaces. > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:117 > + delete m_component; Ditto.
Venkat Penukonda
Comment 10 2011-06-06 11:42:20 PDT
Created attachment 96103 [details] Updated Patch Patch updated to correct the indentation issue. Please review.
Eric Seidel (no email)
Comment 11 2011-12-21 12:02:04 PST
Ping Qt reviewers?
Eric Seidel (no email)
Comment 12 2012-01-30 15:24:07 PST
As far as I can tell the project does not want this patch. Would a Qt person please r- and close?
Simon Hausmann
Comment 13 2012-02-13 22:54:57 PST
Comment on attachment 96103 [details] Updated Patch Venkat, are you still interested in getting this change in?
Venkat Penukonda
Comment 14 2012-02-26 17:30:11 PST
No, not interested as really long time has been passed since I submitted this bug and webkit has gone through many changes since then. This patch is not that important now. I am planning to review and submit qml tests for webkit2 webview api soon anyway in a separate patch.
Note You need to log in before you can comment on or make changes to this bug.