[Oracle] Add oracle importer tests and fix set parsing details (#7215)

* [Oracle] Add oracle importer tests and fix set parsing details

- Add oracle_importer_test and oracle_importer_benchmark_test targets
- Preserve the first printing's legalities when an existing card is reused
- Concatenate split-card coloridentity and sort/dedupe card colors
- Use a raw string for the Basic Land format regex
- Pre-allocate the card hash and micro-optimize string handling

Took 2 minutes

* [Oracle/Tests] Pin cmc coercion in CI run; scope the reserve pass

The #7214 coercion assertion lived only in oracle_importer_benchmark_test,
which gets no add_test and so never runs under ctest. Add NumericManaValueCoercedToCmc
and LegacyConvertedManaCostCoercedToCmc to oracle_importer_test (a CI-ran
binary): manaValue/convertedManaCost are JSON numbers in AllPrintings, and
QJsonValue::toString() would drop them to an empty cmc without the
#7214 coercion fix.

Wrap the distinct-name reserve pass in a bare block so the ~35k name
QStrings are handed back before the memory-heavy import loop starts.

---------

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

oracle/src/oracleimporter.cpp

@@ -8,6 +8,7 @@
 #include <QJsonDocument>
 #include <QJsonObject>
 #include <QRegularExpression>
+#include <QSet>
 #include <algorithm>
 #include <climits>
 #include <libcockatrice/card/database/parser/cockatrice_xml_4.h>
@@ -67,7 +68,8 @@ bool OracleImporter::readSetsFromByteArray(const QByteArray &data)
         // capitalize set type
         if (setType.length() > 0) {
             // basic grammar for words that aren't capitalized, like in "From the Vault"
-            const QStringList noCapitalize = {"the", "a", "an", "on", "to", "for", "of", "in", "and", "with", "or"};
+            static const QStringList noCapitalize = {"the", "a",  "an",  "on",   "to", "for",
+                                                     "of",  "in", "and", "with", "or"};
             QStringList words = setType.split("_");
             setType.clear();
             bool first = false;
@@ -75,7 +77,7 @@ bool OracleImporter::readSetsFromByteArray(const QByteArray &data)
                 if (first && noCapitalize.contains(item)) {
                     setType += item + QString(" ");
                 } else {
-                    setType += item[0].toUpper() + item.mid(1, -1) + QString(" ");
+                    setType += item[0].toUpper() + item.mid(1) + QString(" ");
                     first = true;
                 }
             }
@@ -123,14 +125,8 @@ static void sortAndReduceColors(QString &colors)
     std::sort(colors.begin(), colors.end(),
               [](const QChar a, const QChar b) { return colorOrder.value(a, INT_MAX) < colorOrder.value(b, INT_MAX); });
     // reduce
-    QChar lastChar = '\0';
+    auto last = std::unique(colors.begin(), colors.end());
-    for (int i = 0; i < colors.size(); ++i) {
+    colors.erase(last, colors.end());
-        if (colors.at(i) == lastChar) {
-            colors.remove(i, 1);
-        } else {
-            lastChar = colors.at(i);
-        }
-    }
 }
 
 CardInfoPtr OracleImporter::addCard(QString name,
@@ -186,8 +182,9 @@ CardInfoPtr OracleImporter::addCard(QString name,
 
     // DETECT CARD POSITIONING INFO
 
-    bool landscapeOrientation = properties.value("maintype") == "Battle" || properties.value("layout") == "split" ||
+    QString layoutVal = properties.value("layout");
-                                properties.value("layout") == "planar";
+    bool landscapeOrientation =
+        properties.value("maintype") == "Battle" || layoutVal == "split" || layoutVal == "planar";
 
     // cards that enter the field tapped
     bool cipt = parseCipt(name, text) || landscapeOrientation;
@@ -426,7 +423,8 @@ int OracleImporter::importCardsFromSet(const CardSetPtr &currentSet, const QJson
                 }
             }
 
-            CardInfoPtr newCard = addCard(name + numComponent, text, isToken, properties, relatedCards, printingInfo);
+            CardInfoPtr newCard =
+                addCard(name + numComponent, text, isToken, std::move(properties), relatedCards, printingInfo);
             numCards++;
         }
     }
@@ -459,7 +457,7 @@ int OracleImporter::importCardsFromSet(const CardSetPtr &currentSet, const QJson
                     if (!thisCardPropertyValue.isEmpty() && originalPropertyValue != thisCardPropertyValue) {
                         if (originalPropertyValue.isEmpty()) { // don't create //es if one field is empty
                             properties.insert(prop, thisCardPropertyValue);
-                        } else if (prop == "colors") { // the card is both colors
+                        } else if (prop == "colors" || prop == "coloridentity") { // the card is both colors
                             properties.insert(prop, originalPropertyValue + thisCardPropertyValue);
                         } else if (prop == "maintype") { // don't create maintypes with //es in them
                             continue;
@@ -471,7 +469,7 @@ int OracleImporter::importCardsFromSet(const CardSetPtr &currentSet, const QJson
                 }
             }
         }
-        CardInfoPtr newCard = addCard(name, text, isToken, properties, {}, printingInfo);
+        CardInfoPtr newCard = addCard(name, text, isToken, std::move(properties), {}, printingInfo);
         numCards++;
     }
 
@@ -552,6 +550,23 @@ int OracleImporter::startImport()
 {
     static ICardSetPriorityController *noOpController = new NoopCardSetPriorityController();
 
+    // Pre-allocate the cards hash to avoid rehashing during import. The hash
+    // is keyed by distinct card name rather than by printings: AllPrintings
+    // ships ~100k printings for ~35k names, so reserving the printing count
+    // would overallocate ~3x (against this stack's RAM goal). Collecting
+    // distinct names is cheap — one pass over the already-parsed name fields.
+    {
+        QSet<QString> distinctNames;
+        for (const SetToDownload &curSetToParse : allSets) {
+            for (const QJsonValue &cardValue : curSetToParse.getCards()) {
+                distinctNames.insert(cardValue.toObject().value("name").toString());
+            }
+        }
+        cards.reserve(distinctNames.size());
+        // The set goes out of scope here, handing the ~35k name QStrings back
+        // to the allocator before the (memory-heavy) import loop starts.
+    }
+
     // add an empty set for tokens
     CardSetPtr tokenSet =
         CardSet::newInstance(noOpController, CardSet::TOKENS_SETNAME, tr("Dummy set containing tokens"), "Tokens");

tests/oracle/CMakeLists.txt

@@ -7,3 +7,35 @@ endif()
 target_link_libraries(parse_cipt_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES})
 
 add_test(NAME parse_cipt_test COMMAND parse_cipt_test)
+
+# Oracle importer unit tests
+add_executable(
+  oracle_importer_test ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp ../../oracle/src/parsehelpers.cpp
+                       oracle_importer_test.cpp
+)
+
+if(NOT GTEST_FOUND)
+  add_dependencies(oracle_importer_test gtest)
+endif()
+
+target_link_libraries(
+  oracle_importer_test libcockatrice_card libcockatrice_interfaces Threads::Threads ${GTEST_BOTH_LIBRARIES}
+  ${TEST_QT_MODULES}
+)
+
+add_test(NAME oracle_importer_test COMMAND oracle_importer_test)
+
+# Oracle importer benchmark tests (manual, not run in CI)
+add_executable(
+  oracle_importer_benchmark_test ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp
+                                 ../../oracle/src/parsehelpers.cpp oracle_importer_benchmark_test.cpp
+)
+
+if(NOT GTEST_FOUND)
+  add_dependencies(oracle_importer_benchmark_test gtest)
+endif()
+
+target_link_libraries(
+  oracle_importer_benchmark_test libcockatrice_card libcockatrice_interfaces Threads::Threads ${GTEST_BOTH_LIBRARIES}
+  ${TEST_QT_MODULES}
+)

tests/oracle/oracle_importer_benchmark_test.cpp

@@ -0,0 +1,264 @@
+#include "../../oracle/src/oracleimporter.h"
+
+#include "gtest/gtest.h"
+#include <QDebug>
+#include <QElapsedTimer>
+#include <QJsonArray>
+#include <QJsonDocument>
+#include <QJsonObject>
+#include <libcockatrice/interfaces/noop_card_set_priority_controller.h>
+
+// Helper: build a synthetic MTGJSON-style JSON with the given number of sets and cards per set
+static QByteArray buildSyntheticData(int numSets, int cardsPerSet)
+{
+    QJsonObject dataObj;
+    for (int s = 0; s < numSets; ++s) {
+        QJsonArray cardsArray;
+        for (int c = 0; c < cardsPerSet; ++c) {
+            QJsonObject card;
+            card["name"] = QString("Card %1").arg(s * cardsPerSet + c);
+            card["text"] = "This is a test card with some rules text.";
+            card["layout"] = "normal";
+            card["manaCost"] = "{W}";
+            card["type"] = "Creature — Human";
+            card["power"] = "2";
+            card["toughness"] = "2";
+            card["colors"] = QJsonArray{"W"};
+            card["colorIdentity"] = QJsonArray{"W"};
+            card["types"] = QJsonArray{"Creature"};
+            // Real MTGJSON types: floats and booleans, not strings. This
+            // exercises the QVariant coercion in the property reader.
+            card["convertedManaCost"] = 1.0;
+            card["manaValue"] = 1.0;
+            card["isOnlineOnly"] = false;
+            card["isRebalanced"] = false;
+
+            QJsonObject legalities;
+            legalities["standard"] = "legal";
+            legalities["modern"] = "legal";
+            legalities["legacy"] = "legal";
+            legalities["vintage"] = "legal";
+            legalities["commander"] = "legal";
+            card["legalities"] = legalities;
+
+            QJsonObject identifiers;
+            identifiers["scryfallId"] = QString("id-%1-%2").arg(s).arg(c);
+            card["identifiers"] = identifiers;
+
+            // In AllPrintings, number and rarity are flat fields on the card
+            // object, exactly as set below.
+            card["number"] = QString::number(c + 1);
+            card["rarity"] = "common";
+
+            cardsArray.append(card);
+        }
+
+        QJsonObject setObj;
+        setObj["code"] = QString("T%1").arg(s, 2, 10, QChar('0'));
+        setObj["name"] = QString("Test Set %1").arg(s);
+        setObj["type"] = "expansion";
+        setObj["releaseDate"] = "2024-01-01";
+        setObj["cards"] = cardsArray;
+
+        dataObj[QString("T%1").arg(s, 2, 10, QChar('0'))] = setObj;
+    }
+
+    QJsonObject root;
+    root["data"] = dataObj;
+    return QJsonDocument(root).toJson(QJsonDocument::Compact);
+}
+
+// ============================================================================
+// Import throughput benchmark
+// ============================================================================
+
+TEST(OracleBenchmark, ImportThroughput)
+{
+    static constexpr int numSets = 10;
+    static constexpr int cardsPerSet = 500;
+
+    QByteArray data = buildSyntheticData(numSets, cardsPerSet);
+
+    OracleImporter importer;
+
+    // Phase 1: Parse JSON
+    QElapsedTimer timer;
+    timer.start();
+    bool ok = importer.readSetsFromByteArray(data);
+    ASSERT_TRUE(ok);
+    qint64 parseMs = timer.elapsed();
+
+    // Phase 2: Import cards
+    timer.restart();
+    int importedSets = importer.startImport();
+    qint64 importMs = timer.elapsed();
+
+    int totalImported = 0;
+    for (const auto &card : importer.getCardList()) {
+        Q_UNUSED(card);
+        totalImported++;
+    }
+
+    // The fixture generates globally unique card names, so the expected
+    // counts are exact: a regression here means cards were dropped.
+    ASSERT_EQ(importedSets, numSets);
+    ASSERT_EQ(totalImported, numSets * cardsPerSet);
+    // Real-data probe: numeric convertedManaCost must be coerced to text
+    // (regression for the QJsonValue::toString() reader in #7214).
+    auto probeCard = importer.getCardList().value("Card 0");
+    ASSERT_FALSE(probeCard.isNull());
+    ASSERT_EQ(probeCard->getProperty("cmc"), "1");
+
+    qDebug().noquote()
+        << QString("Oracle Import Benchmark: %1 sets, %2 unique cards").arg(importedSets).arg(totalImported);
+    qDebug().noquote() << QString("  JSON parse:  %1 ms").arg(parseMs);
+    qDebug().noquote() << QString("  Card import: %1 ms").arg(importMs);
+    qDebug().noquote() << QString("  Total:       %1 ms").arg(parseMs + importMs);
+    if (importMs > 0) {
+        qDebug().noquote() << QString("  Throughput:  %1 cards/sec")
+                                  .arg(static_cast<double>(totalImported) / importMs * 1000.0, 0, 'f', 0);
+    }
+}
+
+// ============================================================================
+// readSetsFromByteArray benchmark
+// ============================================================================
+
+TEST(OracleBenchmark, ParseJsonThroughput)
+{
+    static constexpr int numSets = 20;
+    static constexpr int cardsPerSet = 1000;
+
+    QByteArray data = buildSyntheticData(numSets, cardsPerSet);
+
+    // Run 5 iterations and report average
+    static constexpr int iterations = 5;
+    qint64 totalMs = 0;
+
+    for (int i = 0; i < iterations; ++i) {
+        OracleImporter importer;
+        QElapsedTimer timer;
+        timer.start();
+        bool ok = importer.readSetsFromByteArray(data);
+        ASSERT_TRUE(ok);
+        totalMs += timer.elapsed();
+    }
+
+    qint64 avgMs = totalMs / iterations;
+    qDebug().noquote() << QString("Parse Benchmark (%1 iterations): avg %2 ms for %3 sets x %4 cards")
+                              .arg(iterations)
+                              .arg(avgMs)
+                              .arg(numSets)
+                              .arg(cardsPerSet);
+}
+
+// ============================================================================
+// Split card merging benchmark
+// ============================================================================
+
+TEST(OracleBenchmark, SplitCardMerging)
+{
+    static constexpr int numSplitCards = 1000;
+
+    QJsonArray cardsList;
+    for (int i = 0; i < numSplitCards; ++i) {
+        QJsonObject face1;
+        face1["name"] = QString("Fire %1 // Ice %1").arg(i);
+        face1["text"] = "Fire side text.";
+        face1["layout"] = "split";
+        face1["side"] = "a";
+        face1["faceName"] = QString("Fire %1").arg(i);
+        face1["colors"] = QJsonArray{"R"};
+        face1["colorIdentity"] = QJsonArray{"R"};
+        face1["types"] = QJsonArray{"Instant"};
+        face1["manaCost"] = "{R}";
+        face1["legalities"] = QJsonObject{{"standard", "not_legal"}};
+        face1["identifiers"] = QJsonObject{{"scryfallId", QString("f-%1").arg(i)}};
+        face1["number"] = QString::number(i + 1);
+        face1["rarity"] = "uncommon";
+
+        QJsonObject face2;
+        face2["name"] = QString("Fire %1 // Ice %1").arg(i);
+        face2["text"] = "Ice side text.";
+        face2["layout"] = "split";
+        face2["side"] = "b";
+        face2["faceName"] = QString("Ice %1").arg(i);
+        face2["colors"] = QJsonArray{"U"};
+        face2["colorIdentity"] = QJsonArray{"U"};
+        face2["types"] = QJsonArray{"Instant"};
+        face2["manaCost"] = "{U}";
+        face2["legalities"] = QJsonObject{{"standard", "not_legal"}};
+        face2["identifiers"] = QJsonObject{{"scryfallId", QString("i-%1").arg(i)}};
+        face2["number"] = QString::number(i + 1);
+        face2["rarity"] = "uncommon";
+
+        cardsList.append(face1);
+        cardsList.append(face2);
+    }
+
+    NoopCardSetPriorityController controller;
+    OracleImporter importer;
+    CardSetPtr set = CardSet::newInstance(&controller, "TST", "Split Test");
+
+    QElapsedTimer timer;
+    timer.start();
+    int count = importer.importCardsFromSet(set, cardsList);
+    qint64 ms = timer.elapsed();
+
+    ASSERT_EQ(count, numSplitCards);
+    qDebug().noquote() << QString("Split Card Merge Benchmark: %1 cards in %2 ms (%3 cards/sec)")
+                              .arg(count)
+                              .arg(ms)
+                              .arg(ms > 0 ? static_cast<double>(count) / ms * 1000.0 : 0.0, 0, 'f', 0);
+}
+
+// ============================================================================
+// sortAndReduceColors microbenchmark
+// ============================================================================
+
+// We can't call sortAndReduceColors directly (it's static), so we benchmark
+// through importCardsFromSet with color properties.
+
+TEST(OracleBenchmark, ImportCardsWithColors)
+{
+    static constexpr int numCards = 10000;
+
+    NoopCardSetPriorityController controller;
+    OracleImporter importer;
+    CardSetPtr set = CardSet::newInstance(&controller, "TST", "Color Test");
+
+    QJsonArray cardsList;
+    for (int i = 0; i < numCards; ++i) {
+        QJsonObject card;
+        card["name"] = QString("Color Card %1").arg(i);
+        card["text"] = "Rules text.";
+        card["layout"] = "normal";
+        card["manaCost"] = "{W}";
+        card["type"] = "Creature — Human";
+        card["types"] = QJsonArray{"Creature"};
+        card["colors"] = QJsonArray{"B", "R", "G", "W", "U"};
+        card["colorIdentity"] = QJsonArray{"B", "R", "G", "W", "U"};
+        card["number"] = QString::number(i + 1);
+        card["rarity"] = "common";
+        card["legalities"] = QJsonObject{{"standard", "legal"}};
+        card["identifiers"] = QJsonObject{{"scryfallId", QString("c-%1").arg(i)}};
+        cardsList.append(card);
+    }
+
+    QElapsedTimer timer;
+    timer.start();
+    int count = importer.importCardsFromSet(set, cardsList);
+    qint64 ms = timer.elapsed();
+
+    ASSERT_EQ(count, numCards);
+    qDebug().noquote() << QString("Import with Colors Benchmark: %1 cards in %2 ms (%3 cards/sec)")
+                              .arg(count)
+                              .arg(ms)
+                              .arg(ms > 0 ? static_cast<double>(count) / ms * 1000.0 : 0.0, 0, 'f', 0);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}

tests/oracle/oracle_importer_test.cpp

@@ -0,0 +1,552 @@
+#include "../../oracle/src/oracleimporter.h"
+
+#include "gtest/gtest.h"
+#include <QJsonArray>
+#include <QJsonDocument>
+#include <QJsonObject>
+#include <QSet>
+#include <libcockatrice/card/format/format_legality_rules.h>
+#include <libcockatrice/card/set/card_set.h>
+#include <libcockatrice/interfaces/noop_card_set_priority_controller.h>
+
+class OracleImporterTest : public ::testing::Test
+{
+protected:
+    void SetUp() override
+    {
+        controller = new NoopCardSetPriorityController();
+        importer = new OracleImporter();
+        set = CardSet::newInstance(controller, "TST", "Test Set");
+    }
+
+    void TearDown() override
+    {
+        delete importer;
+        delete controller;
+    }
+
+    // Helper: build a minimal card JSON object
+    QJsonObject makeCard(const QString &name,
+                         const QString &colors = "",
+                         const QString &colorIdentity = "",
+                         const QVariantMap &legalities = {})
+    {
+        QJsonObject card;
+        card["name"] = name;
+        card["text"] = "Rules text.";
+        card["layout"] = "normal";
+        card["manaCost"] = "{W}";
+        card["type"] = "Creature — Human";
+        card["types"] = QJsonArray{"Creature"};
+        card["number"] = "1";
+        card["rarity"] = "common";
+
+        if (!colors.isEmpty()) {
+            QJsonArray arr;
+            for (const QChar &c : colors) {
+                arr.append(QString(c));
+            }
+            card["colors"] = arr;
+        }
+        if (!colorIdentity.isEmpty()) {
+            QJsonArray arr;
+            for (const QChar &c : colorIdentity) {
+                arr.append(QString(c));
+            }
+            card["colorIdentity"] = arr;
+        }
+        if (!legalities.isEmpty()) {
+            QJsonObject legalObj;
+            for (auto it = legalities.constBegin(); it != legalities.constEnd(); ++it) {
+                legalObj[it.key()] = it.value().toString();
+            }
+            card["legalities"] = legalObj;
+        }
+
+        QJsonObject identifiers;
+        identifiers["scryfallId"] = QUuid::createUuid().toString(QUuid::WithoutBraces);
+        card["identifiers"] = identifiers;
+
+        return card;
+    }
+
+    NoopCardSetPriorityController *controller;
+    OracleImporter *importer;
+    CardSetPtr set;
+};
+
+// ============================================================================
+// sortAndReduceColors tests (tested via importCardsFromSet)
+// ============================================================================
+
+TEST_F(OracleImporterTest, SortAndReduceColorsSingleColor)
+{
+    QJsonArray cards{makeCard("Red Card", "R", "R")};
+    importer->importCardsFromSet(set, cards);
+
+    auto card = importer->getCardList().value("Red Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("colors"), "R");
+}
+
+TEST_F(OracleImporterTest, SortAndReduceColorsDeduplicates)
+{
+    QJsonArray cards{makeCard("Dedup Card", "WWUUB", "WU")};
+    importer->importCardsFromSet(set, cards);
+
+    auto card = importer->getCardList().value("Dedup Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("colors"), "WUB");
+}
+
+TEST_F(OracleImporterTest, SortAndReduceColorsSortsWUBRG)
+{
+    QJsonArray cards{makeCard("Sort Card", "RGW", "RGW")};
+    importer->importCardsFromSet(set, cards);
+
+    auto card = importer->getCardList().value("Sort Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("colors"), "WRG");
+}
+
+TEST_F(OracleImporterTest, SortAndReduceColorsAllFive)
+{
+    QJsonArray cards{makeCard("Five Color", "BRGWU", "BRGWU")};
+    importer->importCardsFromSet(set, cards);
+
+    auto card = importer->getCardList().value("Five Color");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("colors"), "WUBRG");
+}
+
+TEST_F(OracleImporterTest, SortAndReduceColorIdentity)
+{
+    QJsonArray cards{makeCard("Color Id Card", "W", "GWR")};
+    importer->importCardsFromSet(set, cards);
+
+    auto card = importer->getCardList().value("Color Id Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("coloridentity"), "WRG");
+}
+
+TEST_F(OracleImporterTest, SingleColorNotSorted)
+{
+    QJsonArray cards{makeCard("Single Card", "B", "B")};
+    importer->importCardsFromSet(set, cards);
+
+    auto card = importer->getCardList().value("Single Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("colors"), "B");
+}
+
+// ============================================================================
+// Legality guard tests
+// ============================================================================
+
+TEST_F(OracleImporterTest, NewCardKeepsLegalityProperties)
+{
+    // Verifies that format-* properties survive addCard on a fresh card
+    // (not the combineLegalities guard, which only runs on existing printings).
+    QVariantMap leg;
+    leg["standard"] = "legal";
+    leg["modern"] = "legal";
+    QJsonArray cards{makeCard("Legal Card", "", "", leg)};
+
+    importer->importCardsFromSet(set, cards);
+    auto card = importer->getCardList().value("Legal Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("format-standard"), "legal");
+    ASSERT_EQ(card->getProperty("format-modern"), "legal");
+}
+
+TEST_F(OracleImporterTest, LegalityMergeAllowedWhenCardHasNoLegalities)
+{
+    // First printing carries no legalities at all, so the guard's
+    // `properties.filter(formatRegex).empty()` predicate is true and the
+    // second printing's legalities must be merged in.
+    QJsonArray cards1{makeCard("Unmerged Card")};
+    importer->importCardsFromSet(set, cards1);
+
+    CardSetPtr set2 = CardSet::newInstance(controller, "TS2", "Second Set");
+    QVariantMap leg;
+    leg["standard"] = "legal";
+    QJsonArray cards2{makeCard("Unmerged Card", "", "", leg)};
+    importer->importCardsFromSet(set2, cards2);
+
+    auto card = importer->getCardList().value("Unmerged Card");
+    ASSERT_FALSE(card.isNull());
+    ASSERT_EQ(card->getProperty("format-standard"), "legal");
+}
+
+TEST_F(OracleImporterTest, LegalityGuardPreservesFirstPrinting)
+{
+    // First printing: standard=legal, modern=legal
+    QVariantMap leg1;
+    leg1["standard"] = "legal";
+    leg1["modern"] = "legal";
+    QJsonArray cards1{makeCard("Guarded Card", "", "", leg1)};
+    importer->importCardsFromSet(set, cards1);
+
+    // Second printing: standard=banned, modern=not_legal
+    CardSetPtr set2 = CardSet::newInstance(controller, "TS2", "Second Set");
+    QVariantMap leg2;
+    leg2["standard"] = "banned";
+    leg2["modern"] = "not_legal";
+    QJsonArray cards2{makeCard("Guarded Card", "", "", leg2)};
+    importer->importCardsFromSet(set2, cards2);
+
+    auto card = importer->getCardList().value("Guarded Card");
+    ASSERT_FALSE(card.isNull());
+    // Guard should preserve first printing's legalities
+    ASSERT_EQ(card->getProperty("format-standard"), "legal");
+    ASSERT_EQ(card->getProperty("format-modern"), "legal");
+}
+
+// ============================================================================
+// createDefaultMagicFormats tests
+// ============================================================================
+
+TEST_F(OracleImporterTest, CreateDefaultMagicFormatsContainsExpectedFormats)
+{
+    auto formats = importer->createDefaultMagicFormats();
+    ASSERT_TRUE(formats.contains("standard"));
+    ASSERT_TRUE(formats.contains("modern"));
+    ASSERT_TRUE(formats.contains("legacy"));
+    ASSERT_TRUE(formats.contains("vintage"));
+    ASSERT_TRUE(formats.contains("commander"));
+    ASSERT_TRUE(formats.contains("pauper"));
+    ASSERT_TRUE(formats.contains("pioneer"));
+    ASSERT_TRUE(formats.contains("brawl"));
+    ASSERT_TRUE(formats.contains("historic"));
+    ASSERT_TRUE(formats.contains("timeless"));
+    ASSERT_TRUE(formats.contains("duel"));
+    ASSERT_TRUE(formats.contains("oathbreaker"));
+}
+
+TEST_F(OracleImporterTest, CreateDefaultMagicFormatsSingletonDeckSizes)
+{
+    auto formats = importer->createDefaultMagicFormats();
+    auto commander = formats.value("commander");
+    ASSERT_FALSE(commander.isNull());
+    ASSERT_EQ(commander->minDeckSize, 100);
+    ASSERT_EQ(commander->maxDeckSize, 100);
+    ASSERT_EQ(commander->maxSideboardSize, 15);
+
+    auto brawl = formats.value("brawl");
+    ASSERT_FALSE(brawl.isNull());
+    ASSERT_EQ(brawl->minDeckSize, 60);
+    ASSERT_EQ(brawl->maxDeckSize, 60);
+}
+
+TEST_F(OracleImporterTest, CreateDefaultMagicFormatsVintageHasRestricted)
+{
+    auto formats = importer->createDefaultMagicFormats();
+    auto vintage = formats.value("vintage");
+    ASSERT_FALSE(vintage.isNull());
+    bool hasRestricted = false;
+    for (const auto &ac : vintage->allowedCounts) {
+        if (ac.label == "restricted") {
+            hasRestricted = true;
+            ASSERT_EQ(ac.max, 1);
+        }
+    }
+    ASSERT_TRUE(hasRestricted);
+}
+
+TEST_F(OracleImporterTest, CreateDefaultMagicFormatsRegexMatchesBasicLands)
+{
+    auto formats = importer->createDefaultMagicFormats();
+    auto standard = formats.value("standard");
+    ASSERT_FALSE(standard.isNull());
+    ASSERT_FALSE(standard->exceptions.isEmpty());
+
+    auto &basicLandsException = standard->exceptions.first();
+    ASSERT_FALSE(basicLandsException.conditions.isEmpty());
+
+    auto &condition = basicLandsException.conditions.first();
+    ASSERT_EQ(condition.field, "type");
+    ASSERT_EQ(condition.matchType, "regex");
+
+    // Verify the regex actually works (was broken before: \b = backspace, not word boundary)
+    QRegularExpression regex(condition.value);
+    ASSERT_TRUE(regex.isValid());
+    ASSERT_TRUE(regex.match("Basic Land — Forest").hasMatch());
+    ASSERT_TRUE(regex.match("Basic Snow Land — Mountain").hasMatch());
+    ASSERT_FALSE(regex.match("Creature — Elf Warrior").hasMatch());
+}
+
+TEST_F(OracleImporterTest, CreateDefaultMagicFormatsCaching)
+{
+    // The memoized map returns the same FormatRulesPtr instances, so the
+    // shared pointers must be identical across calls. This is the only
+    // observable effect of the cache: contents would match either way.
+    auto first = importer->createDefaultMagicFormats();
+    auto second = importer->createDefaultMagicFormats();
+    ASSERT_EQ(first.value("standard").data(), second.value("standard").data());
+}
+
+// ============================================================================
+// readSetsFromByteArray tests
+// ============================================================================
+
+TEST_F(OracleImporterTest, ReadSetsFromByteArrayValidJson)
+{
+    QJsonObject setObj;
+    setObj["code"] = "tst";
+    setObj["name"] = "Test Set";
+    setObj["type"] = "expansion";
+    setObj["releaseDate"] = "2024-01-01";
+    setObj["cards"] = QJsonArray();
+
+    QJsonObject root;
+    root["data"] = QJsonObject{{"TST", setObj}};
+
+    QByteArray data = QJsonDocument(root).toJson();
+    ASSERT_TRUE(importer->readSetsFromByteArray(data));
+    ASSERT_EQ(importer->getSets().size(), 1);
+    ASSERT_EQ(importer->getSets().first().getShortName(), "TST");
+}
+
+TEST_F(OracleImporterTest, ReadSetsFromByteArrayInvalidJson)
+{
+    QByteArray data = "not valid json";
+    ASSERT_FALSE(importer->readSetsFromByteArray(data));
+}
+
+TEST_F(OracleImporterTest, ReadSetsFromByteArrayEmptyData)
+{
+    QJsonObject root;
+    root["data"] = QJsonObject();
+
+    QByteArray data = QJsonDocument(root).toJson();
+    ASSERT_FALSE(importer->readSetsFromByteArray(data));
+}
+
+TEST_F(OracleImporterTest, ReadSetsFromByteArrayCapitalizesSetType)
+{
+    QJsonObject setObj;
+    setObj["code"] = "ftv";
+    setObj["name"] = "From The Vault";
+    setObj["type"] = "from_the_vault";
+    setObj["releaseDate"] = "2024-01-01";
+    setObj["cards"] = QJsonArray();
+
+    QJsonObject root;
+    root["data"] = QJsonObject{{"FTV", setObj}};
+
+    QByteArray data = QJsonDocument(root).toJson();
+    ASSERT_TRUE(importer->readSetsFromByteArray(data));
+    ASSERT_EQ(importer->getSets().first().getSetType(), "From the Vault");
+}
+
+TEST_F(OracleImporterTest, ReadSetsFromByteArraySortsSetsByName)
+{
+    // QJsonObject iterates keys in lexicographic order ("AAA" before "ZZZ"),
+    // so leaving the natural order matching the alphabetical sort makes the
+    // assertion pass trivially. Inverting it keeps the sort meaningful:
+    // iteration yields "AAA" (Zeta Set) first, then the sort by name must
+    // promote "ZZZ" (Alpha Set) to the front.
+    QJsonObject setA;
+    setA["code"] = "aaa";
+    setA["name"] = "Zeta Set";
+    setA["type"] = "expansion";
+    setA["releaseDate"] = "2024-01-01";
+    setA["cards"] = QJsonArray();
+
+    QJsonObject setB;
+    setB["code"] = "zzz";
+    setB["name"] = "Alpha Set";
+    setB["type"] = "expansion";
+    setB["releaseDate"] = "2024-01-01";
+    setB["cards"] = QJsonArray();
+
+    QJsonObject root;
+    root["data"] = QJsonObject{{"AAA", setA}, {"ZZZ", setB}};
+
+    QByteArray data = QJsonDocument(root).toJson();
+    ASSERT_TRUE(importer->readSetsFromByteArray(data));
+    auto sets = importer->getSets();
+    ASSERT_GE(sets.size(), 2);
+    ASSERT_EQ(sets.first().getShortName(), "ZZZ");
+}
+
+// ============================================================================
+// Split card coloridentity tests
+// ============================================================================
+
+TEST_F(OracleImporterTest, SplitCardColorIdentityConcatenated)
+{
+    QJsonObject leg{{"standard", "not_legal"}};
+
+    QJsonObject face1;
+    face1["name"] = "Fire // Ice";
+    face1["text"] = "Fire deals 2 damage.";
+    face1["layout"] = "split";
+    face1["side"] = "a";
+    face1["faceName"] = "Fire";
+    face1["colors"] = QJsonArray{"R"};
+    face1["colorIdentity"] = QJsonArray{"R"};
+    face1["types"] = QJsonArray{"Instant"};
+    face1["manaCost"] = "{R}";
+    face1["legalities"] = leg;
+    face1["identifiers"] = QJsonObject{{"scryfallId", "aaa"}};
+    face1["number"] = "1";
+    face1["rarity"] = "uncommon";
+
+    QJsonObject face2;
+    face2["name"] = "Fire // Ice";
+    face2["text"] = "Ice taps target artifact.";
+    face2["layout"] = "split";
+    face2["side"] = "b";
+    face2["faceName"] = "Ice";
+    face2["colors"] = QJsonArray{"U"};
+    face2["colorIdentity"] = QJsonArray{"U"};
+    face2["types"] = QJsonArray{"Instant"};
+    face2["manaCost"] = "{U}";
+    face2["legalities"] = leg;
+    face2["identifiers"] = QJsonObject{{"scryfallId", "bbb"}};
+    face2["number"] = "1";
+    face2["rarity"] = "uncommon";
+
+    QJsonArray cardsList{face1, face2};
+    int count = importer->importCardsFromSet(set, cardsList);
+    ASSERT_EQ(count, 1);
+
+    auto card = importer->getCardList().value("Fire // Ice");
+    ASSERT_FALSE(card.isNull());
+
+    // coloridentity should be "RU" (concatenated), then sorted to "UR"
+    // by sortAndReduceColors when it reaches addCard
+    ASSERT_EQ(card->getProperty("coloridentity"), "UR");
+}
+
+TEST_F(OracleImporterTest, SplitCardColorsConcatenated)
+{
+    QJsonObject leg{{"standard", "not_legal"}};
+
+    QJsonObject face1;
+    face1["name"] = "Fire // Ice";
+    face1["text"] = "Fire deals 2 damage.";
+    face1["layout"] = "split";
+    face1["side"] = "a";
+    face1["faceName"] = "Fire";
+    face1["colors"] = QJsonArray{"R"};
+    face1["colorIdentity"] = QJsonArray{"R"};
+    face1["types"] = QJsonArray{"Instant"};
+    face1["manaCost"] = "{R}";
+    face1["legalities"] = leg;
+    face1["identifiers"] = QJsonObject{{"scryfallId", "aaa"}};
+    face1["number"] = "1";
+    face1["rarity"] = "uncommon";
+
+    QJsonObject face2;
+    face2["name"] = "Fire // Ice";
+    face2["text"] = "Ice taps target artifact.";
+    face2["layout"] = "split";
+    face2["side"] = "b";
+    face2["faceName"] = "Ice";
+    face2["colors"] = QJsonArray{"U"};
+    face2["colorIdentity"] = QJsonArray{"U"};
+    face2["types"] = QJsonArray{"Instant"};
+    face2["manaCost"] = "{U}";
+    face2["legalities"] = leg;
+    face2["identifiers"] = QJsonObject{{"scryfallId", "bbb"}};
+    face2["number"] = "1";
+    face2["rarity"] = "uncommon";
+
+    QJsonArray cardsList{face1, face2};
+    importer->importCardsFromSet(set, cardsList);
+
+    auto card = importer->getCardList().value("Fire // Ice");
+    ASSERT_FALSE(card.isNull());
+
+    QString colors = card->getProperty("colors");
+    ASSERT_FALSE(colors.contains("//")) << "colors should not contain '//', got: " << colors.toStdString();
+    ASSERT_TRUE(colors.contains("R"));
+    ASSERT_TRUE(colors.contains("U"));
+}
+
+// ============================================================================
+// Mana cost formatting tests
+// ============================================================================
+
+TEST_F(OracleImporterTest, ManaCostStripsBraces)
+{
+    QJsonObject card = makeCard("Mana Card");
+    card["manaCost"] = "{2}{W}{B}";
+    QJsonArray cards{card};
+
+    importer->importCardsFromSet(set, cards);
+    auto result = importer->getCardList().value("Mana Card");
+    ASSERT_FALSE(result.isNull());
+    ASSERT_EQ(result->getProperty("manacost"), "2WB");
+}
+
+// cmc comes through as a JSON number ("convertedManaCost"/"manaValue" are
+// floats in AllPrintings), so this pins the number-to-text coercion that
+// QJsonValue::toString() dropped in #7214.
+TEST_F(OracleImporterTest, NumericManaValueCoercedToCmc)
+{
+    QJsonObject card = makeCard("Cmc Card");
+    card["manaValue"] = 3;
+    QJsonArray cards{card};
+
+    importer->importCardsFromSet(set, cards);
+    auto result = importer->getCardList().value("Cmc Card");
+    ASSERT_FALSE(result.isNull());
+    ASSERT_EQ(result->getProperty("cmc"), "3");
+}
+
+TEST_F(OracleImporterTest, LegacyConvertedManaCostCoercedToCmc)
+{
+    QJsonObject card = makeCard("Legacy Cmc Card");
+    card["convertedManaCost"] = 3.0;
+    QJsonArray cards{card};
+
+    importer->importCardsFromSet(set, cards);
+    auto result = importer->getCardList().value("Legacy Cmc Card");
+    ASSERT_FALSE(result.isNull());
+    ASSERT_EQ(result->getProperty("cmc"), "3");
+}
+
+// ============================================================================
+// Card deduplication tests
+// ============================================================================
+
+TEST_F(OracleImporterTest, DuplicateCardNameReturnsExisting)
+{
+    QJsonArray cards{makeCard("Dupe Card")};
+    importer->importCardsFromSet(set, cards);
+
+    CardSetPtr set2 = CardSet::newInstance(controller, "TS2", "Second Set");
+    QJsonArray cards2{makeCard("Dupe Card")};
+    importer->importCardsFromSet(set2, cards2);
+
+    ASSERT_EQ(importer->getCardList().size(), 1);
+}
+
+TEST_F(OracleImporterTest, AELigatureReplaced)
+{
+    QJsonObject card = makeCard(QString::fromUtf8("\xC3\x86ther Vial")); // Æther Vial
+    QJsonArray cards{card};
+
+    importer->importCardsFromSet(set, cards);
+    // Æ is replaced with AE, resulting in "AEther Vial"
+    ASSERT_FALSE(importer->getCardList().contains(QString::fromUtf8("\xC3\x86ther Vial")));
+    ASSERT_TRUE(importer->getCardList().contains("AEther Vial"));
+}
+
+TEST_F(OracleImporterTest, ApostropheNormalized)
+{
+    QJsonObject card = makeCard(QString::fromUtf8("Jace\u2019s Ingenuity"));
+    QJsonArray cards{card};
+
+    importer->importCardsFromSet(set, cards);
+    ASSERT_TRUE(importer->getCardList().contains("Jace's Ingenuity"));
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}