Merge lp:~dobey/unity-scope-click/log-response-body into lp:unity-scope-click/devel

[dobey]

Add logging of the response body on network errors.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:247
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/48/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/23
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/22

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/48/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Please include the error number in the debug message, and the url that caused it since this is asynchronous.
Also, please add some tests.

review: Needs Fixing

[dobey (dobey)]

The error code is already being logged at a higher level. Should I move that into this lower level code?

The network tests are very flaky it seems. I had problems doing this at a higher level, as calling qDebug() would make several tests fail, with the signal not being emitted. I'm not sure if it's an issue with how we're using QObject here, or a deeper problem.

[Alejandro J. Cura (alecu)]

I still think that network_access_manager is the wrong layer to put this, so I've taken a look at the flaky tests when qDebug() was added to click::web::Response::errorHandler:
[ FAILED ] IndexTest.testSearchNetworkErrorIgnored
[ FAILED ] IndexTest.testGetDetailsNetworkErrorReported
[ FAILED ] ReviewsTest.testFetchReviewsNetworkErrorReported
[ FAILED ] WebClientTest.testResponseFailed

The quick fix is to add a new expect line in each failing test, like:
EXPECT_CALL(*reply, readAll()).Times(1).WillOnce(Return("{'fake': 'error'}"));

Furthermore, the first three tests should not be depending on this behaviour: they should not be mocking NetworkReply, but instead should be mocking WebClient. I think the quick fix is enough, though.

review: Needs Fixing

[lp:~dobey/unity-scope-click/log-response-body]

248. By dobey on 2014-05-13

Move the logging back up to the web::Response errorHandler.
Add a default return value for QByteArray in MockNetworkReply.

249. By dobey on 2014-05-13

Remove extraneous include.

250. By dobey on 2014-05-13

Use qWarning instead of qCritical for the error response body logging.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:249
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/52/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/26
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/26/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/26

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/52/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:250
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/53/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/27
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/27/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/27

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/53/rebuild

review: Approve (continuous-integration)

[lp:~dobey/unity-scope-click/log-response-body]

251. By dobey on 2014-05-13

Combine the error string and the response body to a single qWarning.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:251
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/54/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/29
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/28
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/28/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/28

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/54/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Looks good now. Thanks for working on it!

review: Approve

=== modified file 'scope/click/webclient.cpp'
--- scope/click/webclient.cpp	2014-05-01 21:04:23 +0000
+++ scope/click/webclient.cpp	2014-05-13 15:34:51 +0000
@@ -174,7 +174,7 @@
 void click::web::Response::errorHandler(QNetworkReply::NetworkError network_error)
 {
     auto message = reply->errorString() + QString(" (%1)").arg(network_error);
-    qDebug() << "emitting error: " << message;
+    qWarning() << "Network error:" << message << "\n" << reply->readAll();
     emit error(message);
 }
 

=== modified file 'scope/tests/mock_network_access_manager.h'
--- scope/tests/mock_network_access_manager.h	2014-02-27 21:26:21 +0000
+++ scope/tests/mock_network_access_manager.h	2014-05-13 15:34:51 +0000
@@ -44,6 +44,12 @@
     void sendError();
 
 public:
+    MockNetworkReply()
+    {
+        // Set a default value for QByteArray-returning mocked methods.
+        ::testing::DefaultValue<QByteArray>::Set(QByteArray(""));
+    }
+
     MOCK_METHOD0(abort, void());
     MOCK_METHOD0(readAll, QByteArray());
     MOCK_METHOD1(attribute, QVariant(QNetworkRequest::Attribute));

=== modified file 'scope/tests/test_webclient.cpp'
--- scope/tests/test_webclient.cpp	2014-05-01 21:04:23 +0000
+++ scope/tests/test_webclient.cpp	2014-05-13 15:34:51 +0000
@@ -309,13 +309,17 @@
     EXPECT_CALL(nam, sendCustomRequest(_, _, _))
             .Times(1)
             .WillOnce(Return(replyPtr));
-    EXPECT_CALL(*reply, errorString()).Times(1).WillOnce(Return("fake error"));
+    EXPECT_CALL(*reply, errorString())
+        .Times(1)
+        .WillOnce(Return("fake error"));
+    EXPECT_CALL(*reply, readAll())
+        .Times(1)
+        .WillOnce(Return("{'error': 'Invalid request.'}"));
 
     auto response = wc.call(FAKE_SERVER + FAKE_PATH);
     QObject::connect(response.data(), &click::web::Response::error,
-                     [this, &response](QString desc){
+                     [this](QString desc){
                          errorHandler(desc);
-                         Q_UNUSED(response);
                      });
 
     EXPECT_CALL(*this, errorHandler(_));