[Tests] Use QtTest for model contracts and signal spies (#7630)

Qt6::Test was linked into every test but never used. Add a small RAII
QAbstractItemModelTester helper and apply it to the deck list model, the card
database model and display proxy, and the sets model. Add QSignalSpy coverage
for the card-added and download-settings signals, and assert explicitly that a
card row is a leaf with no child columns (the tester does not check that).

The contract checker immediately found four model bugs, fixed here:
- DeckListModel allowed children below non-zero columns.
- CardDatabaseModel reported every card as a child of every card row.
- CardDatabaseModel reported six columns for a valid (leaf) card index.
- CardDatabaseDisplayModel claimed more rows were fetchable while rowCount
  already exposed them all, so fetchMore() inserted duplicates.

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/interface/widgets/visual_database_display/visual_database_display_widget.cpp

@@ -417,11 +417,6 @@ void VisualDatabaseDisplayWidget::loadNextPage()
     int start = currentPage * cardsPerPage;
     int end = qMin(start + cardsPerPage, rowCount);
 
-    // Load more cards if we are at the end of the current list and can fetch more
-    if (end >= rowCount && databaseDisplayModel->canFetchMore(QModelIndex())) {
-        databaseDisplayModel->fetchMore(QModelIndex());
-    }
-
     // Load the next page of cards and add them to the flow widget
     flowWidget->setUpdatesEnabled(false);
     loadPage(start, end);

libcockatrice_models/libcockatrice/models/database/card_database_display_model.cpp

@@ -33,8 +33,6 @@ CardDatabaseDisplayModel::CardDatabaseDisplayModel(QObject *parent)
         invalidate();
         emit modelDirty();
     });
-
-    loadedRowCount = 0;
 }
 
 void CardDatabaseDisplayModel::setSourceModel(QAbstractItemModel *model)
@@ -45,10 +43,7 @@ void CardDatabaseDisplayModel::setSourceModel(QAbstractItemModel *model)
 
     connect(model, &QAbstractItemModel::rowsRemoved, this, [this]() { dirty(); });
 
-    connect(model, &QAbstractItemModel::modelReset, this, [this]() {
-        loadedRowCount = 0;
-        dirty();
-    });
+    connect(model, &QAbstractItemModel::modelReset, this, [this]() { dirty(); });
 }
 
 QMap<wchar_t, wchar_t> CardDatabaseDisplayModel::characterTranslation = {{L'“', L'\"'},
@@ -56,32 +51,6 @@ QMap<wchar_t, wchar_t> CardDatabaseDisplayModel::characterTranslation = {{L'“'
                                                                          {L'‘', L'\''},
                                                                          {L'’', L'\''}};
 
-bool CardDatabaseDisplayModel::canFetchMore(const QModelIndex &index) const
-{
-    return loadedRowCount < sourceModel()->rowCount(index);
-}
-
-void CardDatabaseDisplayModel::fetchMore(const QModelIndex &index)
-{
-    int remainder = sourceModel()->rowCount(index) - loadedRowCount;
-    int itemsToFetch = qMin(100, remainder);
-
-    if (itemsToFetch == 0) {
-        return;
-    }
-
-    const auto startIndex = qMin(rowCount(QModelIndex()), loadedRowCount);
-    beginInsertRows(QModelIndex(), startIndex, startIndex + itemsToFetch - 1);
-
-    loadedRowCount += itemsToFetch;
-    endInsertRows();
-}
-
-int CardDatabaseDisplayModel::rowCount(const QModelIndex &parent) const
-{
-    return QSortFilterProxyModel::rowCount(parent);
-}
-
 bool CardDatabaseDisplayModel::lessThan(const QModelIndex &left, const QModelIndex &right) const
 {
     const QString leftString = sourceModel()->data(left, CardDatabaseModel::SortRole).toString();

libcockatrice_models/libcockatrice/models/database/card_database_display_model.h

@@ -42,7 +42,6 @@ private:
     QSet<QString> cardNameSet, cardTypes, cardColors;
     FilterTree *filterTree;
     FilterString *filterString;
-    int loadedRowCount;
     QTimer dirtyTimer;
     CardSearchLanguage searchLanguage;
     QString searchText;
@@ -85,9 +84,6 @@ public:
         dirtyTimer.start(20);
     }
     void clearFilterAll();
-    [[nodiscard]] int rowCount(const QModelIndex &parent = QModelIndex()) const override;
-    [[nodiscard]] bool canFetchMore(const QModelIndex &parent) const override;
-    void fetchMore(const QModelIndex &parent) override;
 signals:
     void modelDirty();
 

libcockatrice_models/libcockatrice/models/database/card_database_model.cpp

@@ -27,14 +27,18 @@ CardDatabaseModel::CardDatabaseModel(CardDatabase *_db, bool _showOnlyCardsFromE
 
 CardDatabaseModel::~CardDatabaseModel() = default;
 
-int CardDatabaseModel::rowCount(const QModelIndex & /*parent*/) const
+int CardDatabaseModel::rowCount(const QModelIndex &parent) const
 {
-    return cardList.size();
+    // A list model's card rows are leaves; returning the full card count for a
+    // valid parent makes every row claim all cards as children.
+    return parent.isValid() ? 0 : cardList.size();
 }
 
-int CardDatabaseModel::columnCount(const QModelIndex & /*parent*/) const
+int CardDatabaseModel::columnCount(const QModelIndex &parent) const
 {
-    return CARDDBMODEL_COLUMNS;
+    // Card rows are leaves, so a valid parent holds no columns. Reporting the
+    // row width for a card index would claim child cells that do not exist.
+    return parent.isValid() ? 0 : CARDDBMODEL_COLUMNS;
 }
 
 QVariant CardDatabaseModel::data(const QModelIndex &index, int role) const

libcockatrice_models/libcockatrice/models/deck_list/deck_list_model.cpp

@@ -134,6 +134,13 @@ void DeckListModel::rebuildTree()
 
 int DeckListModel::rowCount(const QModelIndex &parent) const
 {
+    // Only column 0 carries children. Column 1 and later indexes are siblings
+    // used by views for the remaining cells of a row, and a model that hangs
+    // children off them breaks QAbstractItemModelTester and proxy models.
+    if (parent.isValid() && parent.column() != 0) {
+        return 0;
+    }
+
     // debugIndexInfo("rowCount", parent);
     auto *node = getNode<InnerDecklistNode *>(parent);
     if (node) {
@@ -282,7 +289,7 @@ QVariant DeckListModel::headerData(const int section, const Qt::Orientation orie
 QModelIndex DeckListModel::index(int row, int column, const QModelIndex &parent) const
 {
     // debugIndexInfo("index", parent);
-    if (!hasIndex(row, column, parent)) {
+    if ((parent.isValid() && parent.column() != 0) || !hasIndex(row, column, parent)) {
         return {};
     }
 

tests/CMakeLists.txt

@@ -516,6 +516,7 @@ add_subdirectory(carddatabase)
 add_subdirectory(deck_list_model)
 add_subdirectory(deck_list_zones)
 add_subdirectory(loading_from_clipboard)
+add_subdirectory(model_contract)
 add_subdirectory(movecard_tests)
 add_subdirectory(oracle)
 add_subdirectory(settings)

tests/model_contract/CMakeLists.txt

@@ -0,0 +1,17 @@
+cockatrice_add_test(
+  NAME
+  model_contract_test
+  SOURCES
+  ${VERSION_STRING_CPP}
+  model_contract_test.cpp
+  LIBS
+  libcockatrice_models
+  libcockatrice_card
+  libcockatrice_deck_list
+  libcockatrice_settings
+  INCLUDE_DIRS
+  ${CMAKE_CURRENT_SOURCE_DIR}/../support
+  ${CMAKE_CURRENT_SOURCE_DIR}/../carddatabase
+  DEFINITIONS
+  CARDDB_DATADIR="${CMAKE_SOURCE_DIR}/tests/carddatabase/data/"
+)

tests/model_contract/model_contract_test.cpp

@@ -0,0 +1,137 @@
+#include "libcockatrice/deck_list/deck_list.h"
+#include "libcockatrice/deck_list/tree/inner_deck_list_node.h"
+#include "libcockatrice/models/database/card_database_display_model.h"
+#include "libcockatrice/models/database/card_database_model.h"
+#include "libcockatrice/models/database/card_set/card_sets_model.h"
+#include "libcockatrice/models/deck_list/deck_list_model.h"
+#include "model_contract_checker.h"
+#include "test_card_database_path_provider.h"
+
+#include "gtest/gtest.h"
+#include <QAbstractItemModel>
+#include <QCoreApplication>
+#include <QModelIndex>
+#include <QSharedPointer>
+#include <QSignalSpy>
+#include <QString>
+#include <QTemporaryDir>
+#include <QVariant>
+#include <libcockatrice/card/card_info.h>
+#include <libcockatrice/card/database/card_database.h>
+#include <libcockatrice/card/game_specific_terms.h>
+#include <libcockatrice/card/printing/exact_card.h>
+#include <libcockatrice/interfaces/noop_card_preference_provider.h>
+#include <libcockatrice/interfaces/noop_card_set_priority_controller.h>
+#include <libcockatrice/settings/download_settings.h>
+
+namespace
+{
+QSharedPointer<DeckList> makeDeck()
+{
+    return QSharedPointer<DeckList>(new DeckList());
+}
+
+void loadCardDatabase(CardDatabase &database)
+{
+    database.loadCardDatabases();
+    ASSERT_EQ(database.getLoadStatus(), Ok);
+}
+
+TEST(ModelContract, DeckListModelKeepsContractWhileZonesAndCardsChange)
+{
+    const QSharedPointer<DeckList> deck = makeDeck();
+    DeckListModel model(nullptr, deck);
+
+    test_support::ModelContractChecker checker(&model);
+
+    ASSERT_NE(deck->getTree()->addCustomZone(DECK_ZONE_MAIN, "Removal"), nullptr);
+    model.rebuildTree();
+
+    CardInfoPtr bolt = CardInfo::newInstance("Lightning Bolt");
+    const QModelIndex added = model.addCard(ExactCard(bolt), DECK_ZONE_MAIN);
+    ASSERT_TRUE(added.isValid());
+
+    CardInfoPtr bears = CardInfo::newInstance("Grizzly Bears");
+    bears->setProperty(Mtg::MainCardType, "Creature");
+    const QModelIndex grouped = model.addCard(ExactCard(bears), "Removal");
+    ASSERT_TRUE(grouped.isValid());
+
+    ASSERT_TRUE(model.removeRows(grouped.row(), 1, grouped.parent()));
+}
+
+TEST(ModelContract, DeckListModelEmitsCardAddedSignal)
+{
+    const QSharedPointer<DeckList> deck = makeDeck();
+    DeckListModel model(nullptr, deck);
+    QSignalSpy spy(&model, &DeckListModel::cardAddedAt);
+
+    const QModelIndex added = model.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+
+    ASSERT_TRUE(added.isValid());
+    EXPECT_EQ(spy.count(), 1);
+}
+
+TEST(ModelContract, CardDatabaseModelAndDisplayProxyKeepContract)
+{
+    CardDatabase database(nullptr, new NoopCardPreferenceProvider(), new TestCardDatabasePathProvider(),
+                          new NoopCardSetPriorityController());
+    ASSERT_NO_FATAL_FAILURE(loadCardDatabase(database));
+
+    CardDatabaseModel source(&database, false);
+    test_support::ModelContractChecker sourceChecker(&source);
+
+    CardDatabaseDisplayModel display;
+    display.setSourceModel(&source);
+    test_support::ModelContractChecker displayChecker(&display);
+
+    ASSERT_GT(source.rowCount(), 0);
+    EXPECT_EQ(display.rowCount(), source.rowCount());
+
+    // A card row is a leaf, so its index must not claim child columns. The
+    // tester does not check this, hence the explicit assertion.
+    const QModelIndex sourceCard = source.index(0, 0);
+    ASSERT_TRUE(sourceCard.isValid());
+    EXPECT_EQ(source.columnCount(sourceCard), 0);
+    const QModelIndex displayCard = display.index(0, 0);
+    ASSERT_TRUE(displayCard.isValid());
+    EXPECT_EQ(display.columnCount(displayCard), 0);
+
+    QSignalSpy dirtySpy(&display, &CardDatabaseDisplayModel::modelDirty);
+    display.setCardName("Do");
+    ASSERT_TRUE(dirtySpy.wait());
+    EXPECT_GT(display.rowCount(), 0);
+    EXPECT_LT(display.rowCount(), source.rowCount());
+}
+
+TEST(ModelContract, SetsModelKeepsContract)
+{
+    CardDatabase database(nullptr, new NoopCardPreferenceProvider(), new TestCardDatabasePathProvider(),
+                          new NoopCardSetPriorityController());
+    ASSERT_NO_FATAL_FAILURE(loadCardDatabase(database));
+
+    SetsModel model(&database);
+    test_support::ModelContractChecker checker(&model);
+
+    ASSERT_GT(model.rowCount(), 0);
+    ASSERT_GT(model.columnCount(), 0);
+}
+
+TEST(ModelContract, SettingsSignalIsObservableWithQSignalSpy)
+{
+    QTemporaryDir tempDir;
+    ASSERT_TRUE(tempDir.isValid());
+    DownloadSettings settings(tempDir.path() + "/", nullptr);
+    QSignalSpy spy(&settings, &DownloadSettings::downloadSpoilerStatusChanged);
+
+    settings.setDownloadSpoilerStatus(true);
+
+    ASSERT_EQ(spy.count(), 1);
+}
+} // namespace
+
+int main(int argc, char **argv)
+{
+    QCoreApplication app(argc, argv);
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}

tests/support/model_contract_checker.h

@@ -0,0 +1,35 @@
+#ifndef COCKATRICE_TEST_MODEL_CONTRACT_CHECKER_H
+#define COCKATRICE_TEST_MODEL_CONTRACT_CHECKER_H
+
+#include <QAbstractItemModel>
+#include <QAbstractItemModelTester>
+#include <memory>
+
+namespace test_support
+{
+/**
+ * @brief Keeps a QAbstractItemModelTester alive for the lifetime of a scope.
+ *
+ * The tester only observes the model while it exists, so construct this before
+ * the mutations a test wants to validate and keep it in scope until the end.
+ * Fatal reporting mode aborts on the first contract violation; a warning would
+ * let the test pass while printing a line nobody reads in CI.
+ */
+class ModelContractChecker
+{
+public:
+    explicit ModelContractChecker(QAbstractItemModel *model)
+        : tester(
+              std::make_unique<QAbstractItemModelTester>(model, QAbstractItemModelTester::FailureReportingMode::Fatal))
+    {
+    }
+
+    ModelContractChecker(const ModelContractChecker &) = delete;
+    ModelContractChecker &operator=(const ModelContractChecker &) = delete;
+
+private:
+    std::unique_ptr<QAbstractItemModelTester> tester;
+};
+} // namespace test_support
+
+#endif // COCKATRICE_TEST_MODEL_CONTRACT_CHECKER_H