Bug 61304 – [Qt] QML WebView API tests need improvements

• Component: WebKit Qt
• Status: RESOLVED WONTFIX

[Venkat Penukonda]

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.

[Venkat Penukonda]

Created attachment 94486 [details]
Proposed patch

[Alexis Menard (darktears)]

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]

Created attachment 94697 [details]
Modified patch

[Venkat Penukonda]

Updated the patch as per above suggestion from Alexis.

[Simon Hausmann]

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]

Created attachment 95019 [details]
Updated patch 

Simon's suggestions above are incorporated into this updated patch.

[Alexis Menard (darktears)]

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]

Created attachment 95203 [details]
Patch updated and resubmitted incorporating the corrections suggested by the reviewer above.

[Alexis Menard (darktears)]

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]

Created attachment 96103 [details]
Updated Patch

Patch updated to correct the indentation issue. Please review.

[Eric Seidel (no email)]

Ping Qt reviewers?

[Eric Seidel (no email)]

As far as I can tell the project does not want this patch.  Would a Qt person please r- and close?

[Simon Hausmann]

Comment on attachment 96103 [details]
Updated Patch

Venkat, are you still interested in getting this change in?

[Venkat Penukonda]

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.