Improve FolderStatusModel tests

src/gui/folderstatusmodel.cpp

@@ -100,6 +100,7 @@ void FolderStatusModel::setAccountState(const AccountState *accountState)
     }
 
     endResetModel();
+    // TODO: maybe we need to analyze the previous state of _dirty before signal emitting?
     emit dirtyChanged();
 
     // Automatically fetch the subfolders to prevent showing the expansion chevron if there are no subfolders
@@ -114,7 +115,7 @@ void FolderStatusModel::setAccountState(const AccountState *accountState)
 
 Qt::ItemFlags FolderStatusModel::flags(const QModelIndex &index) const
 {
-    if (!_accountState) {
+    if (!_accountState || !index.isValid()) {
         return {};
     }
 
@@ -157,10 +158,12 @@ QVariant FolderStatusModel::data(const QModelIndex &index, int role) const
         if (role == FolderStatusDelegate::AddButton) {
             return true;
         } else if (role == Qt::ToolTipRole) {
-            if (!_accountState->isConnected()) {
-                return tr("You need to be connected to add a folder");
+            if (_accountState) {
+                if (!_accountState->isConnected()) {
+                    return tr("You need to be connected to add a folder");
+                }
+                return tr("Click this button to add a folder to synchronize.");
             }
-            return tr("Click this button to add a folder to synchronize.");
         }
         return {};
     }
@@ -393,6 +396,8 @@ bool FolderStatusModel::setData(const QModelIndex &index, const QVariant &value,
                 }
             }
         }
+        // TODO: think about that maybe better to introduce a separate setter for [_dirty] var with emitting of signal if changed
+        // TODO: maybe we need to analyze the previous state of _dirty before signal emitting?
         _dirty = true;
         emit dirtyChanged();
         emit dataChanged(index, index, QVector<int>() << role);
@@ -416,25 +421,21 @@ int FolderStatusModel::rowCount(const QModelIndex &parent) const
         }
         return _folders.count() + 1; // +1 for the "add folder" button
     }
-    const auto info = infoForIndex(parent);
-    if (!info)
-        return 0;
-    if (info->hasLabel())
-        return 1;
-    return info->_subs.count();
+    if (const auto info = infoForIndex(parent)) {
+        return info->hasLabel() ? 1 : info->_subs.count();
+    }
+    return 0;
 }
 
 FolderStatusModel::ItemType FolderStatusModel::classify(const QModelIndex &index) const
 {
-    if (const auto sub = static_cast<SubFolderInfo *>(index.internalPointer())) {
-        if (sub->hasLabel()) {
-            return FetchLabel;
-        } else {
-            return SubFolder;
+    if (index.isValid()) {
+        if (const auto sub = static_cast<const SubFolderInfo *>(index.internalPointer())) {
+            return sub->hasLabel() ? FetchLabel : SubFolder;
+        }
+        if (index.row() < _folders.count()) {
+            return RootFolder;
         }
-    }
-    if (index.row() < _folders.count()) {
-        return RootFolder;
     }
     return AddButton;
 }

test/testfolderstatusmodel.cpp

@@ -15,6 +15,7 @@
 #include <QtTest>
 #include <QStandardPaths>
 #include <QAbstractItemModelTester>
+#include <theme.h>
 
 #include "accountstate.h"
 #include "folderman.h"
@@ -29,7 +30,6 @@ using namespace OCC::Utility;
 class TestFolderStatusModel : public QObject
 {
     Q_OBJECT
-
     std::unique_ptr<FolderMan> _folderMan;
 
 public:
@@ -44,8 +44,103 @@ private Q_SLOTS:
 
         _folderMan.reset(new FolderMan{});
     }
+
+    void testThatFolderManHasInstance() {
+        QVERIFY2(nullptr != FolderMan::instance(), "folder manager must not be a NULL");
+    }
+
+    void testEmptyModelHasOneColumn()
+    {
+        const FolderStatusModel test;
+        QCOMPARE(test.columnCount(), 1);
+    }
+
+    void testAccountStateIsConnected()
+    {
+        const auto fakeFolder = makeFakeFolder();
+        const FakeAccountState accountState{fakeFolder.account()};
+        QVERIFY2(accountState.isConnected(), "account must be connected");
+    }
+
+    void testAbilityToAddRemoveAccountFolders()
+    {
+        executeFolderManTest([](const AccountState *accountState, Folder *folder){
+            QVERIFY2(folder, "folder manager was not able to add folder for a given account");
+            QCOMPARE(folder->accountState(), accountState);
+        });
+    }
+
+    void testModelConsistencyWithFakeFolder()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            test.setAccountState(accountState);
+            QVERIFY2(!test.isDirty(), "model is dirty after set account state");
+        });
+    }
+
+    void testNewModelEmitDirtyChangedSignalAfterSetAccountState()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            const QSignalSpy dirtySignalSpy{&test, &FolderStatusModel::dirtyChanged};
+            test.setAccountState(accountState);
+            QCOMPARE(dirtySignalSpy.count(), 1);
+        });
+    }
+
+    void testModelRowsCount()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            test.setAccountState(accountState);
+            // folders count +1 for the "add folder" button if [singleSyncFolder] is true
+            const auto expectedRowsCount = Theme::instance()->singleSyncFolder() ? 1 : 2;
+            QCOMPARE(test.rowCount(), expectedRowsCount);
+        });
+    }
+
+    void testRowsChildrenConsistency()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            test.setAccountState(accountState);
+            QVERIFY2(test.hasChildren({}), "empty index always has children");
+            // we assume that folder may contains of some child elements until not yet fetched
+            QCOMPARE(test.hasChildren(test.index(0)), !test._folders.front()._fetched);
+        });
+    }
+
+    void testThatRowClassifyWorksProperly()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            test.setAccountState(accountState);
+            QCOMPARE(test.classify({}), FolderStatusModel::ItemType::AddButton);
+            QCOMPARE(test.classify(test.index(0)), FolderStatusModel::ItemType::RootFolder);
+        });
+    }
+
+    void testSubFoldersInfoForIndices()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            test.setAccountState(accountState);
+            QCOMPARE(test.infoForIndex({}), nullptr);
+            QVERIFY2(nullptr != test.infoForIndex(test.index(0)), "NULL info for index 0");
+        });
+    }
+
+    void testThatAppropriateSignalsEmittedIfItemChecked()
+    {
+        executeConsistencyModelTest([](const AccountState *accountState, FolderStatusModel &test) {
+            test.setAccountState(accountState);
+            // set checked for our single root folder
+            const QSignalSpy dataChangedSignalSpy{&test, &QAbstractItemModel::dataChanged};
+            const QSignalSpy dirtySignalSpy{&test, &FolderStatusModel::dirtyChanged};
+            QVERIFY(test.setData(test.index(0), Qt::CheckState::Checked, Qt::CheckStateRole));
+            QVERIFY2(dataChangedSignalSpy.count() > 0, "make sure the signal [dataChanged] was emitted at least one time");
+            QVERIFY2(dirtySignalSpy.count() > 0, "make sure the signal [dirtyChanged] was emitted at least one time");
+            QVERIFY2(test.isDirty(), "model is not dirty after set checked state for the folder");
+        });
+    }
 
-    void startModel()
+private:
+    static FakeFolder makeFakeFolder()
     {
         FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
 
@@ -59,21 +154,51 @@ private Q_SLOTS:
         account->setCapabilities(capabilities);
         account->setCredentials(new FakeCredentials{fakeFolder.networkAccessManager()});
         account->setUrl(QUrl(("owncloud://somehost/owncloud")));
-        auto accountState = FakeAccountState(account);
-        QVERIFY(accountState.isConnected());
-
-        auto folderDef = folderDefinition(fakeFolder.localPath());
-        folderDef.targetPath = "";
-        const auto folder = FolderMan::instance()->addFolder(&accountState, folderDef);
-        QVERIFY(folder);
-
-        FolderStatusModel test;
-        test.setAccountState(&accountState);
-
-        QSKIP("Initial test implementation is known to be broken");
-        QAbstractItemModelTester modeltester(&test);
-
-        test.fetchMore({});
+        return fakeFolder;
+    }
+
+    static OCC::FolderDefinition makeFolderDefinition(const QString &path) {
+        auto folderDef = folderDefinition(path);
+        folderDef.targetPath.clear();
+        return folderDef;
+    }
+
+    static OCC::FolderDefinition makeFolderDefinition(const FakeFolder &folder) {
+        return makeFolderDefinition(folder.localPath());
+    }
+
+    template <typename TestFunctor>
+    static void executeFolderManTest(TestFunctor&& functor)
+    {
+        const auto fakeFolder = makeFakeFolder();
+        const AccountStatePtr accountState{new FakeAccountState{fakeFolder.account()}};
+        const auto folder = FolderMan::instance()->addFolder(accountState.get(),
+                                                             makeFolderDefinition(fakeFolder));
+        try {
+            functor(accountState.get(), folder);
+            // cleanup
+            if (folder) {
+                FolderMan::instance()->removeFolder(folder);
+            }
+        }
+        catch(...) {
+            // cleanup
+            if (folder) {
+                FolderMan::instance()->removeFolder(folder);
+            }
+            throw; // rethrow exception from test framework
+        }
+    }
+
+    template <typename TestFunctor>
+    static void executeConsistencyModelTest(TestFunctor&& functor)
+    {
+        executeFolderManTest([functor](const AccountState *accountState, Folder*){
+            FolderStatusModel test;
+            QAbstractItemModelTester tester{&test, QAbstractItemModelTester::FailureReportingMode::QtTest};
+            tester.setUseFetchMore(true);
+            functor(accountState, test);
+        });
     }
 };