[Webkit-unassigned] [Bug 61304] [Qt] QML WebView API tests need improvements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61304


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94697|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #5 from Simon Hausmann <hausmann at webkit.org>  2011-05-25 17:37:43 PST ---
(From update of attachment 94697)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.