Merge pull request #790 from DonLakeFlyer/BetterFileMgrUt

More comprehensive testing of FileManager error/edge cases

src/qgcunittest/MockMavlinkFileServer.cc

@@ -23,25 +23,33 @@
 
 #include "MockMavlinkFileServer.h"
 
+const MockMavlinkFileServer::ErrorMode_t MockMavlinkFileServer::rgFailureModes[] = {
+    MockMavlinkFileServer::errModeNoResponse,
+    MockMavlinkFileServer::errModeNakResponse,
+    MockMavlinkFileServer::errModeNoSecondResponse,
+    MockMavlinkFileServer::errModeNakSecondResponse,
+    MockMavlinkFileServer::errModeBadCRC,
+};
+const size_t MockMavlinkFileServer::cFailureModes = sizeof(MockMavlinkFileServer::rgFailureModes) / sizeof(MockMavlinkFileServer::rgFailureModes[0]);
+
 const MockMavlinkFileServer::FileTestCase MockMavlinkFileServer::rgFileTestCases[MockMavlinkFileServer::cFileTestCases] = {
     // File fits one Read Ack packet, partially filling data
-    { "partial.qgc",    sizeof(((QGCUASFileManager::Request*)0)->data) - 1 },
+    { "partial.qgc",    sizeof(((QGCUASFileManager::Request*)0)->data) - 1,     false },
     // File fits one Read Ack packet, exactly filling all data
-    { "exact.qgc",      sizeof(((QGCUASFileManager::Request*)0)->data) },
+    { "exact.qgc",      sizeof(((QGCUASFileManager::Request*)0)->data),         true },
     // File is larger than a single Read Ack packets, requires multiple Reads
-    { "multi.qgc",      sizeof(((QGCUASFileManager::Request*)0)->data) + 1 },
+    { "multi.qgc",      sizeof(((QGCUASFileManager::Request*)0)->data) + 1,     true },
 };
 
 // We only support a single fixed session
 const uint8_t MockMavlinkFileServer::_sessionId = 1;
 
-MockMavlinkFileServer::MockMavlinkFileServer(void)
+MockMavlinkFileServer::MockMavlinkFileServer(void) :
+    _errMode(errModeNone)
 {
 
 }
 
-
-
 /// @brief Handles List command requests. Only supports root folder paths.
 ///         File list returned is set using the setFileList method.
 void MockMavlinkFileServer::_listCommand(QGCUASFileManager::Request* request)
@@ -83,6 +91,13 @@ void MockMavlinkFileServer::_listCommand(QGCUASFileManager::Request* request)
         }
 
         _emitResponse(&ackResponse);
+    } else if (_errMode == errModeNakSecondResponse) {
+        // Nak error all subsequent requests
+        _sendNak(QGCUASFileManager::kErrPerm);
+        return;
+    } else if (_errMode == errModeNoSecondResponse) {
+        // No response for all subsequent requests
+        return;
     } else {
         // FIXME: Does not support directories that span multiple packets
         _sendNak(QGCUASFileManager::kErrEOF);
@@ -136,22 +151,26 @@ void MockMavlinkFileServer::_readCommand(QGCUASFileManager::Request* request)
     uint32_t readOffset = request->hdr.offset;  // offset into file for reading
     uint8_t cDataBytes = 0;                     // current number of data bytes used
     
+    if (readOffset != 0) {
+        // If we get here it means the client is requesting additional data past the first request
+        if (_errMode == errModeNakSecondResponse) {
+            // Nak error all subsequent requests
+            _sendNak(QGCUASFileManager::kErrPerm);
+            return;
+        } else if (_errMode == errModeNoSecondResponse) {
+            // No rsponse for all subsequent requests
+            return;
+        }
+    }
+    
     if (readOffset >= _readFileLength) {
         _sendNak(QGCUASFileManager::kErrEOF);
         return;
     }
     
-    // Write length byte if needed
-    if (readOffset == 0) {
-        response.data[0] = _readFileLength;
-        readOffset++;
-        cDataBytes++;
-    }
-    
     // Write file bytes. Data is a repeating sequence of 0x00, 0x01, .. 0xFF.
     for (; cDataBytes < sizeof(response.data) && readOffset < _readFileLength; readOffset++, cDataBytes++) {
-        // Subtract one from readOffset to take into account length byte and start file data a 0x00
-        response.data[cDataBytes] = (readOffset - 1) & 0xFF;
+        response.data[cDataBytes] = readOffset & 0xFF;
     }
     
     // We should always have written something, otherwise there is something wrong with the code above
@@ -187,6 +206,15 @@ void MockMavlinkFileServer::sendMessage(mavlink_message_t message)
     QGCUASFileManager::Request ackResponse;
 
     Q_ASSERT(message.msgid == MAVLINK_MSG_ID_ENCAPSULATED_DATA);
+    
+    if (_errMode == errModeNoResponse) {
+        // Don't respond to any requests, this shold cause the client to eventually timeout waiting for the ack
+        return;
+    } else if (_errMode == errModeNakResponse) {
+        // Nak all requests, the actual error send back doesn't really matter as long as it's an error
+        _sendNak(QGCUASFileManager::kErrPerm);
+        return;
+    }
 
     mavlink_encapsulated_data_t requestEncapsulatedData;
     mavlink_msg_encapsulated_data_decode(&message, &requestEncapsulatedData);
@@ -280,6 +308,10 @@ void MockMavlinkFileServer::_emitResponse(QGCUASFileManager::Request* request)
     mavlink_message_t   mavlinkMessage;
     
     request->hdr.crc32 = QGCUASFileManager::crc32(request);
+    if (_errMode == errModeBadCRC) {
+        // Return a bad CRC
+        request->hdr.crc32++;
+    }
     
     mavlink_msg_encapsulated_data_pack(250, MAV_COMP_ID_IMU, &mavlinkMessage, 0 /*_encdata_seq*/, (uint8_t*)request);
     

src/qgcunittest/MockMavlinkFileServer.h

@@ -46,18 +46,46 @@ public:
     /// to indicate (F)ile or (D)irectory.
     void setFileList(QStringList& fileList) { _fileList = fileList; }
     
+    /// @brief By calling setErrorMode with one of these modes you can cause the server to simulate an error.
+    typedef enum {
+        errModeNone,                ///< No error, respond correctly
+        errModeNoResponse,          ///< No response to any request, client should eventually time out with no Ack
+        errModeNakResponse,         ///< Nak all requests
+        errModeNoSecondResponse,    ///< No response to subsequent request to initial command
+        errModeNakSecondResponse,   ///< Nak subsequent request to initial command
+        errModeBadCRC,              ///< Return response with bad CRC
+        errModeBadSequence          ///< Return response with bad sequence number, NYI: Waiting on Firmware sequence # support
+    } ErrorMode_t;
+    
+    /// @brief Sets the error mode for command responses. This allows you to simulate various server errors.
+    void setErrorMode(ErrorMode_t errMode) { _errMode = errMode; };
+    
+    /// @brief Array of failure modes you can cycle through for testing. By looping through this array you can avoid
+    /// hardcoding the specific error modes in your unit test. This way when new error modes are added your unit test
+    /// code may not need to be modified.
+    static const ErrorMode_t rgFailureModes[];
+    
+    /// @brief The number of ErrorModes in the rgFailureModes array.
+    static const size_t cFailureModes;
+    
     // From MockMavlinkInterface
     virtual void sendMessage(mavlink_message_t message);
     
+    /// @brief Used to represent a single test case for download testing.
     struct FileTestCase {
-        const char* filename;
-        uint8_t     length;
+        const char* filename;               ///< Filename to download
+        uint8_t     length;                 ///< Length of file in bytes
+        bool        fMultiPacketResponse;   ///< true: multiple acks required to download, false: single ack contains entire download
     };
     
+    /// @brief The numbers of test cases in the rgFileTestCases array.
     static const size_t cFileTestCases = 3;
+    
+    /// @brief The set of files supported by the mock server for testing purposes. Each one represents a different edge case for testing.
     static const FileTestCase rgFileTestCases[cFileTestCases];
     
 signals:
+    /// @brief You can connect to this signal to be notified when the server receives a Terminate command.
     void terminateCommandReceived(void);
     
 private:
@@ -72,7 +100,8 @@ private:
     QStringList _fileList;  ///< List of files returned by List command
     
     static const uint8_t    _sessionId;
-    uint8_t                 _readFileLength; ///< Length of active file being read
+    uint8_t                 _readFileLength;    ///< Length of active file being read
+    ErrorMode_t             _errMode;           ///< Currently set error mode, as specified by setErrorMode
 };
 
 #endif

src/qgcunittest/QGCUASFileManagerTest.cc

@@ -51,6 +51,10 @@ void QGCUASFileManagerUnitTest::init(void)
     _fileManager = new QGCUASFileManager(NULL, &_mockUAS);
     Q_CHECK_PTR(_fileManager);
     
+    // Reset any internal state back to normal
+    _mockFileServer.setErrorMode(MockMavlinkFileServer::errModeNone);
+    _fileListReceived.clear();
+    
     bool connected = connect(&_mockFileServer, SIGNAL(messageReceived(LinkInterface*, mavlink_message_t)), _fileManager, SLOT(receiveMessage(LinkInterface*, mavlink_message_t)));
     Q_ASSERT(connected);
     Q_UNUSED(connected);    // Silent release build compiler warning
@@ -98,6 +102,12 @@ void QGCUASFileManagerUnitTest::_ackTest(void)
     // we don't get any error signals.
     QVERIFY(_fileManager->_sendCmdTestAck());
     QVERIFY(_multiSpy->checkNoSignals());
+    
+    // Setup for no response from ack. This should cause a timeout error;
+    _mockFileServer.setErrorMode(MockMavlinkFileServer::errModeNoResponse);
+    QVERIFY(_fileManager->_sendCmdTestAck());
+    QTest::qWait(_ackTimerTimeoutMsecs); // Let the file manager timeout
+    QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask), true);
 }
 
 void QGCUASFileManagerUnitTest::_noAckTest(void)
@@ -108,7 +118,7 @@ void QGCUASFileManagerUnitTest::_noAckTest(void)
     
     // This should not get the ack back and timeout.
     QVERIFY(_fileManager->_sendCmdTestNoAck());
-    QTest::qWait(2000); // Let the file manager timeout, magic number 2 secs must be larger than file manager ack timeout
+    QTest::qWait(_ackTimerTimeoutMsecs); // Let the file manager timeout
     QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask), true);
 }
 
@@ -137,21 +147,51 @@ void QGCUASFileManagerUnitTest::_listTest(void)
     QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask | resetStatusMessagesSignalMask), true);
     _multiSpy->clearAllSignals();
 
-    // Send a list command at the root of the directory tree
-    //  We should get back a single resetStatusMessages signal
-    //  We should not get back an errorMessage signal
-    //  We should get back one or more statusMessage signals
-    //  The returned list should match out inputs
-    
+    // Setup the mock file server with a valid directory list
     QStringList fileList;
     fileList << "Ddir" << "Ffoo" << "Fbar";
     _mockFileServer.setFileList(fileList);
     
-    _fileListReceived.clear();
+    // Run through the various server side failure modes
+    for (size_t i=0; i<MockMavlinkFileServer::cFailureModes; i++) {
+        MockMavlinkFileServer::ErrorMode_t errMode = MockMavlinkFileServer::rgFailureModes[i];
+        qDebug() << "Testing failure mode:" << errMode;
+        _mockFileServer.setErrorMode(errMode);
+        
+        _fileManager->listDirectory("/");
+        QTest::qWait(_ackTimerTimeoutMsecs); // Let the file manager timeout
+        
+        if (errMode == MockMavlinkFileServer::errModeNoSecondResponse || errMode == MockMavlinkFileServer::errModeNakSecondResponse) {
+            // For simulated server errors on subsequent Acks, the first Ack will go through. This means we should have gotten some
+            // partial results. In the case of the directory list test set, all entries fit into the first ack, so we should have
+            // gotten back all of them.
+            QCOMPARE(_multiSpy->getSpyByIndex(statusMessageSignalIndex)->count(), fileList.count());
+            _multiSpy->clearSignalByIndex(statusMessageSignalIndex);
+            
+            // And then it should have errored out because the next list Request would have failed.
+            QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask | resetStatusMessagesSignalMask), true);
+        } else {
+            // For the simulated errors which failed the intial response we should not have gotten any results back at all.
+            // Just an error.
+            QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask | resetStatusMessagesSignalMask), true);
+        }
+
+        // Set everything back to initial state
+        _fileListReceived.clear();
+        _multiSpy->clearAllSignals();
+        _mockFileServer.setErrorMode(MockMavlinkFileServer::errModeNone);
+    }
+
+    // Send a list command at the root of the directory tree which should succeed
+    //  We should get back a single resetStatusMessages signal
+    //  We should not get back an errorMessage signal
+    //  We should get back a statusMessage signal for each entry
+    //  The returned list should match our inputs
     
     _fileManager->listDirectory("/");
-    QCOMPARE(_multiSpy->checkSignalByMask(resetStatusMessagesSignalMask), true);  // We should be told to reset status messages
-    QCOMPARE(_multiSpy->checkNoSignalByMask(errorMessageSignalMask), true);  // We should not get an error signals
+    QCOMPARE(_multiSpy->checkSignalByMask(resetStatusMessagesSignalMask), true);
+    QCOMPARE(_multiSpy->checkNoSignalByMask(errorMessageSignalMask), true);
+    QCOMPARE(_multiSpy->getSpyByIndex(statusMessageSignalIndex)->count(), fileList.count());
     QVERIFY(_fileListReceived == fileList);
 }
 
@@ -167,13 +207,10 @@ void QGCUASFileManagerUnitTest::_validateFileContents(const QString& filePath, u
     QByteArray bytes = file.readAll();
     file.close();
     
-    // Validate length byte
-    QCOMPARE((uint8_t)bytes[0], length);
-    
     // Validate file contents:
     //      Repeating 0x00, 0x01 .. 0xFF until file is full
-    for (uint8_t i=1; i<bytes.length(); i++) {
-        QCOMPARE((uint8_t)bytes[i], (uint8_t)((i-1) & 0xFF));
+    for (uint8_t i=0; i<bytes.length(); i++) {
+        QCOMPARE((uint8_t)bytes[i], (uint8_t)(i & 0xFF));
     }
 }
 
@@ -197,14 +234,61 @@ void QGCUASFileManagerUnitTest::_openTest(void)
             Q_ASSERT(QFile::remove(filePath));
         }
     }
-
-    // Run through the set of file test cases
     
-    // We setup a spy on the terminate command signal so that we can determine that a Terminate command was
+    // We setup a spy on the Terminate command signal so that we can determine that a Terminate command was
     // correctly sent after the Open/Read commands complete.
     QSignalSpy terminateSpy(&_mockFileServer, SIGNAL(terminateCommandReceived()));
     
+    // Run through the set of file test cases
     for (size_t i=0; i<MockMavlinkFileServer::cFileTestCases; i++) {
+        
+        // Run through the various failure modes for this test case
+        for (size_t j=0; j<MockMavlinkFileServer::cFailureModes; j++) {
+            MockMavlinkFileServer::ErrorMode_t errMode = MockMavlinkFileServer::rgFailureModes[j];
+            qDebug() << "Testing failure mode:" << errMode;
+            _mockFileServer.setErrorMode(errMode);
+            
+            _fileManager->downloadPath(MockMavlinkFileServer::rgFileTestCases[i].filename, QDir::temp());
+            QTest::qWait(_ackTimerTimeoutMsecs); // Let the file manager timeout
+            
+            if (errMode == MockMavlinkFileServer::errModeNoSecondResponse || errMode == MockMavlinkFileServer::errModeNakSecondResponse) {
+                // For simulated server errors on subsequent Acks, the first Ack will go through. We must handle things differently depending
+                // on whether the downloaded file requires multiple packets to complete the download.
+                if (MockMavlinkFileServer::rgFileTestCases[i].fMultiPacketResponse) {
+                    // The downloaded file requires multiple Acks to complete. Hence second Read should have failed.
+                    QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask | resetStatusMessagesSignalMask), true);
+
+                    // Open command succeeded, so we should get a Terminate for the open
+                    QCOMPARE(terminateSpy.count(), 1);
+                } else {
+                    // The downloaded file fits within a single Ack response, hence there is no second Read issued.
+                    // This should result in a successful download.
+                    
+                    //  We should get a single resetStatusMessages signal
+                    //  We should get a single statusMessage signal, which indicates download completion
+                    QCOMPARE(_multiSpy->checkOnlySignalByMask(statusMessageSignalMask | resetStatusMessagesSignalMask), true);
+                    
+                    // We should get a single Terminate command to close the Open session
+                    QCOMPARE(terminateSpy.count(), 1);
+                    
+                    // Validate file contents
+                    QString filePath = QDir::temp().absoluteFilePath(MockMavlinkFileServer::rgFileTestCases[i].filename);
+                    _validateFileContents(filePath, MockMavlinkFileServer::rgFileTestCases[i].length);
+                }
+            } else {
+                // For all the other simulated server errors the Open command should have failed. Since the Open failed
+                // there is no session to terminate, hence no Terminate in this case.
+                QCOMPARE(_multiSpy->checkOnlySignalByMask(errorMessageSignalMask | resetStatusMessagesSignalMask), true);
+                QCOMPARE(terminateSpy.count(), 0);
+            }
+
+            // Cleanup for next iteration
+            _multiSpy->clearAllSignals();
+            terminateSpy.clear();
+            _mockFileServer.setErrorMode(MockMavlinkFileServer::errModeNone);
+        }
+
+        // Run what should be a successful file download test case. No servers errors are being simulated.
         _fileManager->downloadPath(MockMavlinkFileServer::rgFileTestCases[i].filename, QDir::temp());
 
         //  We should get a single resetStatusMessages signal
@@ -212,10 +296,11 @@ void QGCUASFileManagerUnitTest::_openTest(void)
         QCOMPARE(_multiSpy->checkOnlySignalByMask(statusMessageSignalMask | resetStatusMessagesSignalMask), true);
         _multiSpy->clearAllSignals();
         
-        // We should get a single Terminate command
+        // We should get a single Terminate command to close the session
         QCOMPARE(terminateSpy.count(), 1);
         terminateSpy.clear();
         
+        // Validate file contents
         QString filePath = QDir::temp().absoluteFilePath(MockMavlinkFileServer::rgFileTestCases[i].filename);
         _validateFileContents(filePath, MockMavlinkFileServer::rgFileTestCases[i].length);
     }

src/qgcunittest/QGCUASFileManagerTest.h

@@ -87,6 +87,10 @@ private:
     static const size_t _cSignals = maxSignalIndex;
     const char*         _rgSignals[_cSignals];
     
+    /// @brief This is the amount of time to wait to allow the FileManager enough time to timeout waiting for an Ack.
+    /// As such it must be larger than the Ack Timeout used by the FileManager.
+    static const int _ackTimerTimeoutMsecs = QGCUASFileManager::ackTimerTimeoutMsecs * 2;
+    
     QStringList _fileListReceived;
 };
 

src/uas/QGCUASFileManager.cc

@@ -261,7 +261,13 @@ void QGCUASFileManager::receiveMessage(LinkInterface* link, mavlink_message_t me
     mavlink_msg_encapsulated_data_decode(&message, &data);
     Request* request = (Request*)&data.data[0];
 
-    // FIXME: Check CRC
+    // Make sure we have a good CRC
+    if (request->hdr.crc32 != crc32(request)) {
+        _currentOperation = kCOIdle;
+        _emitErrorMessage(tr("Bad CRC on received message"));
+        return;
+    }
+
 
     if (request->hdr.opcode == kRspAck) {
 
@@ -289,7 +295,7 @@ void QGCUASFileManager::receiveMessage(LinkInterface* link, mavlink_message_t me
                 break;
 
             default:
-                _emitErrorMessage("Ack received in unexpected state");
+                _emitErrorMessage(tr("Ack received in unexpected state"));
                 break;
         }
     } else if (request->hdr.opcode == kRspNak) {
@@ -462,7 +468,7 @@ void QGCUASFileManager::_setupAckTimeout(void)
     Q_ASSERT(!_ackTimer.isActive());
 
     _ackTimer.setSingleShot(true);
-    _ackTimer.start(_ackTimerTimeoutMsecs);
+    _ackTimer.start(ackTimerTimeoutMsecs);
 }
 
 /// @brief Clears the ack timeout timer

src/uas/QGCUASFileManager.h

@@ -39,6 +39,10 @@ public:
     bool _sendCmdTestAck(void) { return _sendOpcodeOnlyCmd(kCmdNone, kCOAck); };
     bool _sendCmdTestNoAck(void) { return _sendOpcodeOnlyCmd(kCmdTestNoAck, kCOAck); };
     bool _sendCmdReset(void) { return _sendOpcodeOnlyCmd(kCmdReset, kCOAck); };
+    
+    /// @brief Timeout in msecs to wait for an Ack time come back. This is public so we can write unit tests which wait long enough
+    /// for the FileManager to timeout.
+    static const int ackTimerTimeoutMsecs = 1000;
 
 signals:
     void statusMessage(const QString& msg);
@@ -140,9 +144,8 @@ protected:
     static quint32 crc32(Request* request, unsigned state = 0);
     static QString errorString(uint8_t errorCode);
 
-    OperationState      _currentOperation;              ///> Current operation of state machine
-    QTimer              _ackTimer;                      ///> Used to signal a timeout waiting for an ack
-    static const int    _ackTimerTimeoutMsecs = 1000;   ///> Timeout in msecs for ack timer
+    OperationState  _currentOperation;              ///> Current operation of state machine
+    QTimer          _ackTimer;                      ///> Used to signal a timeout waiting for an ack
     
     UASInterface* _mav;
     quint16 _encdata_seq;