[Libreoffice-commits] online.git: test/helpers.hpp test/TileCacheTests.cpp

[Ashod Nakashian]

 test/TileCacheTests.cpp |   71 +++++++++++++++++++++++++++++++++---------------
 test/helpers.hpp        |    2 -
 2 files changed, 51 insertions(+), 22 deletions(-)

New commits:
commit 7dad1c5f6a64445bbefa2821e06beda8716bbc05
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Sun Jan 1 23:18:44 2017 -0500

    wsd: more forgiving canceltiles unittests
    
    canceltiles is not guaranteed to prevent
    tiles from getting sent back to the client.
    
    There is an obvious race between receiving
    tile requets and cancelling them.
    
    This reduces failures by repeating the
    unittest when there is spurious failure.
    
    Change-Id: Icf299e2212e175dc4c0cbc1d2f91c37725f2b261
    Reviewed-on: https://gerrit.libreoffice.org/32631
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/test/TileCacheTests.cpp b/test/TileCacheTests.cpp
index 30897a0..df8e336 100644
--- a/test/TileCacheTests.cpp
+++ b/test/TileCacheTests.cpp
@@ -236,13 +236,30 @@ void TileCacheTests::testPerformance()
 void TileCacheTests::testCancelTiles()
 {
     const auto testName = "cancelTiles ";
-    auto socket = loadDocAndGetSocket("setclientpart.ods", _uri, testName);
 
-    // Request a huge tile, and cancel immediately.
-    sendTextFrame(socket, "tilecombine part=0 width=2560 height=2560 tileposx=0 tileposy=0 tilewidth=38400 tileheight=38400");
-    sendTextFrame(socket, "canceltiles");
+    // The tile response can race past the canceltiles,
+    // so be forgiving to avoid spurious failures.
+    const size_t repeat = 4;
+    for (size_t i = 1; i <= repeat; ++i)
+    {
+        std::cerr << "cancelTiles try #" << i << std::endl;
+
+        auto socket = loadDocAndGetSocket("setclientpart.ods", _uri, testName);
 
-    assertNotInResponse(socket, "tile:", testName);
+        // Request a huge tile, and cancel immediately.
+        sendTextFrame(socket, "tilecombine part=0 width=2560 height=2560 tileposx=0 tileposy=0 tilewidth=38400 tileheight=38400");
+        sendTextFrame(socket, "canceltiles");
+
+        const auto res = getResponseString(socket, "tile:", testName, 1000);
+        if (res.empty())
+        {
+            break;
+        }
+        else if (i == repeat)
+        {
+            CPPUNIT_ASSERT_MESSAGE("Did not expect getting message [" + res + "].", res.empty());
+        }
+    }
 }
 
 void TileCacheTests::testCancelTilesMultiView()
@@ -250,27 +267,39 @@ void TileCacheTests::testCancelTilesMultiView()
     std::string documentPath, documentURL;
     getDocumentPathAndURL("setclientpart.ods", documentPath, documentURL);
 
-    auto socket1 = loadDocAndGetSocket(_uri, documentURL, "cancelTilesMultiView-1 ");
-    auto socket2 = loadDocAndGetSocket(_uri, documentURL, "cancelTilesMultiView-2 ", true);
+    // The tile response can race past the canceltiles,
+    // so be forgiving to avoid spurious failures.
+    const size_t repeat = 4;
+    for (size_t j = 1; j <= repeat; ++j)
+    {
+        std::cerr << "cancelTilesMultiView try #" << j << std::endl;
 
-    sendTextFrame(socket1, "tilecombine part=0 width=256 height=256 tileposx=0,3840,7680,11520,0,3840,7680,11520 tileposy=0,0,0,0,3840,3840,3840,3840 tilewidth=3840 tileheight=3840", "cancelTilesMultiView-1 ");
-    sendTextFrame(socket2, "tilecombine part=0 width=256 height=256 tileposx=0,3840,7680,0 tileposy=0,0,0,22520 tilewidth=3840 tileheight=3840", "cancelTilesMultiView-2 ");
+        // Request a huge tile, and cancel immediately.
+        auto socket1 = loadDocAndGetSocket(_uri, documentURL, "cancelTilesMultiView-1 ");
+        auto socket2 = loadDocAndGetSocket(_uri, documentURL, "cancelTilesMultiView-2 ", true);
 
-    sendTextFrame(socket1, "canceltiles");
+        sendTextFrame(socket1, "tilecombine part=0 width=256 height=256 tileposx=0,3840,7680,11520,0,3840,7680,11520 tileposy=0,0,0,0,3840,3840,3840,3840 tilewidth=3840 tileheight=3840", "cancelTilesMultiView-1 ");
+        sendTextFrame(socket2, "tilecombine part=0 width=256 height=256 tileposx=0,3840,7680,0 tileposy=0,0,0,22520 tilewidth=3840 tileheight=3840", "cancelTilesMultiView-2 ");
 
-    for (auto i = 0; i < 4; ++i)
-    {
-        getTileMessage(*socket2, "cancelTilesMultiView-2 ");
-    }
+        sendTextFrame(socket1, "canceltiles");
+        auto res = getResponseString(socket1, "tile:", "cancelTilesMultiView-1 ", 1000);
+        if (!res.empty() && j == repeat)
+        {
+            CPPUNIT_ASSERT_MESSAGE("Did not expect getting message [" + res + "].", res.empty());
+        }
 
-    // FIXME: Note that especially when this is run on a loaded machine, the server might not honor
-    // the 'canceltiles' but still send out a tile, or it has already sent the tile before it even
-    // gets the 'canceltiles'. That is not an error. It is a bit silly to have it cause an assertion
-    // failure here. Transient failures make a unit test worse than no unit test. Should we remove
-    // this testCancelTilesMultiView altogether?
+        for (auto i = 0; i < 4; ++i)
+        {
+            getTileMessage(*socket2, "cancelTilesMultiView-2 ");
+        }
 
-    assertNotInResponse(socket1, "tile:", "cancelTilesMultiView-1 ");
-    assertNotInResponse(socket2, "tile:", "cancelTilesMultiView-2 ");
+        // Should never get more than 4 tiles on socket2.
+        assertNotInResponse(socket2, "tile:", "cancelTilesMultiView-2 ");
+        if (res.empty())
+        {
+            break;
+        }
+    }
 }
 
 void TileCacheTests::testUnresponsiveClient()
diff --git a/test/helpers.hpp b/test/helpers.hpp
index 61820a0..6c70fdd 100644
--- a/test/helpers.hpp
+++ b/test/helpers.hpp
@@ -315,7 +315,7 @@ template <typename T>
 std::string assertNotInResponse(T& ws, const std::string& prefix, const std::string name = "")
 {
     const auto res = getResponseString(ws, prefix, name, 1000);
-    CPPUNIT_ASSERT_MESSAGE("Did not expect getting message [" + res + "].", res.empty());
+    CPPUNIT_ASSERT_MESSAGE(name + "Did not expect getting message [" + res + "].", res.empty());
     return res;
 }