From 0f3c9782e6e2459f3fea361962492f52fa9c4fd9 Mon Sep 17 00:00:00 2001
From: Timur Pocheptsov <timur.pocheptsov@qt.io>
Date: Mon, 20 Nov 2017 16:50:12 +0100
Subject: tst_QNetworkReply::ioHttpRedirectErrors - fix a flaky test
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This test became a real pain recently. A close look at the test shows
several problems (strangely enough, the failure can never be
reproduced on real machines, only on VM - Ubuntu and RHEL 6.6).
There are several asserts that are firing from time to time here and
there. They show that the logic in test is broken/incorrect. QNAM can
open several connections to a host, our test then incorrectly resets
its 'client' data-member and bad things can later happen after
'bytesWrittenSlot' executed (and deleted a socket). For example,
I can reproduce this scenario in every second run:

1. incoming connection -> client = socket(descriptor), connect to
   client's readyRead (s1)
2. incoming connection -> client = socket(descriptor), connect to
   client's readyRead (s2)
QNAM sends a request on s1. We reply on s2 (which is already wrong)
and call client->deleteLater(), which resets client to nullptr.
If QNAM sends something else on s1, we hit assert(!client.isNull()).

To avoid this, whenever 'sender' in any slot is different from the
'client', we use the actual 'sender' to reply. Another problem is this
weird and rather cryptic waitForFinish which is not needed in this
particular test since we wait for reply error, not 'finished'.

As it happened before - it's not clear if these two problems
were the cause of guaranteed fails on CI - an integration failed
~10 times in a row in the same test (not happening anymore though).

Task-number: QTBUG-64569
Change-Id: Id9aa091290350c61fadf1c3c001e7c2e1b5ac8f4
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
---
 .../access/qnetworkreply/tst_qnetworkreply.cpp     | 38 +++++++++++++++-------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
index 3a752c0748..9fa54597f1 100644
--- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
+++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
@@ -622,7 +622,7 @@ protected:
         Q_ASSERT(!client.isNull());
         // we need to emulate the bytesWrittenSlot call if the data is empty.
         if (dataToTransmit.size() == 0) {
-            QMetaObject::invokeMethod(this, "bytesWrittenSlot", Qt::QueuedConnection);
+            emit client->bytesWritten(0);
         } else {
             client->write(dataToTransmit);
             // FIXME: For SSL connections, if we don't flush the socket, the
@@ -659,22 +659,26 @@ private slots:
 #ifndef QT_NO_SSL
     void slotSslErrors(const QList<QSslError>& errors)
     {
-        Q_ASSERT(!client.isNull());
-        qDebug() << "slotSslErrors" << client->errorString() << errors;
+        QTcpSocket *currentClient = qobject_cast<QTcpSocket *>(sender());
+        Q_ASSERT(currentClient);
+        qDebug() << "slotSslErrors" << currentClient->errorString() << errors;
     }
 #endif
     void slotError(QAbstractSocket::SocketError err)
     {
-        if (client.isNull())
-            qDebug() << "slotError" << err;
-        else
-            qDebug() << "slotError" << err << client->errorString();
+        QTcpSocket *currentClient = qobject_cast<QTcpSocket *>(sender());
+        Q_ASSERT(currentClient);
+        qDebug() << "slotError" << err << currentClient->errorString();
     }
 
 public slots:
     void readyReadSlot()
     {
-        Q_ASSERT(!client.isNull());
+        QTcpSocket *currentClient = qobject_cast<QTcpSocket *>(sender());
+        Q_ASSERT(currentClient);
+        if (currentClient != client)
+            client = currentClient;
+
         receivedData += client->readAll();
         const int doubleEndlPos = receivedData.indexOf("\r\n\r\n");
 
@@ -8290,11 +8294,23 @@ void tst_QNetworkReply::ioHttpRedirectErrors()
     QNetworkReplyPtr reply(manager.get(request));
     if (localhost.scheme() == "https")
         reply.data()->ignoreSslErrors();
-    QSignalSpy spy(reply.data(), SIGNAL(error(QNetworkReply::NetworkError)));
 
-    QCOMPARE(waitForFinish(reply), int(Failure));
+    QEventLoop eventLoop;
+    QTimer watchDog;
+    watchDog.setSingleShot(true);
 
-    QCOMPARE(spy.count(), 1);
+    reply->connect(reply.data(), QOverload<QNetworkReply::NetworkError>().of(&QNetworkReply::error),
+                   [&eventLoop](QNetworkReply::NetworkError){
+                        eventLoop.exit(Failure);
+                   });
+
+    watchDog.connect(&watchDog, &QTimer::timeout, [&eventLoop](){
+                        eventLoop.exit(Timeout);
+                    });
+
+    watchDog.start(5000);
+
+    QCOMPARE(eventLoop.exec(), int(Failure));
     QCOMPARE(reply->error(), error);
 }
 
-- 
cgit v1.2.3