diff --git a/oracle/src/oracleimporter.cpp b/oracle/src/oracleimporter.cpp index c7ae3f2f8..f0dc3b1ae 100644 --- a/oracle/src/oracleimporter.cpp +++ b/oracle/src/oracleimporter.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -142,6 +143,8 @@ CardInfoPtr OracleImporter::addCard(QString name, if (existingIt != cards.constEnd()) { CardInfoPtr card = existingIt.value(); card->addToSet(printingInfo.getSet(), printingInfo); + // Only merge legalities when the card has none yet, so multi-format + // printings don't overwrite each other's legality lists. if (card->getProperties().filter(formatRegex).empty()) { card->combineLegalities(properties); } @@ -222,7 +225,10 @@ CardInfoPtr OracleImporter::addCard(QString name, static QString getJsonString(const QJsonObject &obj, const QString &key) { - return obj.value(key).toString(); + // QVariant coerces numbers and booleans to text, while QJsonValue::toString() + // returns a null string for them — some MTGJSON fields (manaValue, + // convertedManaCost, isOnlineOnly, isRebalanced) carry those types. + return obj.value(key).toVariant().toString(); } int OracleImporter::importCardsFromSet(const CardSetPtr ¤tSet, const QJsonArray &cardsList) @@ -327,12 +333,18 @@ int OracleImporter::importCardsFromSet(const CardSetPtr ¤tSet, const QJson allNameProps.insert(faceName); // special handling properties - QString colors = card.value("colors").toVariant().toStringList().join(""); + QString colors; + for (const QJsonValue &color : card.value("colors").toArray()) { + colors += color.toString(); + } if (!colors.isEmpty()) { properties.insert("colors", colors); } - QString colorIdentity = card.value("colorIdentity").toVariant().toStringList().join(""); + QString colorIdentity; + for (const QJsonValue &color : card.value("colorIdentity").toArray()) { + colorIdentity += color.toString(); + } if (!colorIdentity.isEmpty()) { properties.insert("coloridentity", colorIdentity); } @@ -528,7 +540,7 @@ static FormatRulesNameMap buildDefaultMagicFormats() return defaultFormatRulesNameMap; } -FormatRulesNameMap OracleImporter::createDefaultMagicFormats() +const FormatRulesNameMap &OracleImporter::createDefaultMagicFormats() { static const FormatRulesNameMap cached = buildDefaultMagicFormats(); return cached; @@ -538,12 +550,18 @@ int OracleImporter::startImport() { static ICardSetPriorityController *noOpController = new NoopCardSetPriorityController(); - // Pre-allocate cards hash to avoid rehashing during import - int estimatedCards = 0; + // 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 distinctNames; for (const SetToDownload &curSetToParse : allSets) { - estimatedCards += curSetToParse.getCards().size(); + for (const QJsonValue &cardValue : curSetToParse.getCards()) { + distinctNames.insert(cardValue.toObject().value("name").toString()); + } } - cards.reserve(estimatedCards); + cards.reserve(distinctNames.size()); // add an empty set for tokens CardSetPtr tokenSet = diff --git a/oracle/src/oracleimporter.h b/oracle/src/oracleimporter.h index 696e35cab..52a7cd349 100644 --- a/oracle/src/oracleimporter.h +++ b/oracle/src/oracleimporter.h @@ -157,7 +157,10 @@ public: int startImport(); bool saveToFile(const QString &fileName, const QString &sourceUrl, const QString &sourceVersion); int importCardsFromSet(const CardSetPtr ¤tSet, const QJsonArray &cardsList); - FormatRulesNameMap createDefaultMagicFormats(); + /** + * @brief Returns the default format rules. The result is memoized on first use and must be treated as immutable. + */ + const FormatRulesNameMap &createDefaultMagicFormats(); const CardNameMap &getCardList() const { return cards; diff --git a/tests/oracle/CMakeLists.txt b/tests/oracle/CMakeLists.txt index 0088f37c9..cbff4f19c 100644 --- a/tests/oracle/CMakeLists.txt +++ b/tests/oracle/CMakeLists.txt @@ -25,6 +25,7 @@ target_link_libraries( add_test(NAME oracle_importer_test COMMAND oracle_importer_test) +# Oracle importer benchmark tests (manual, not run in CI, incl. RAM benchmark) # Optional compression libs, mirrored from oracle/CMakeLists.txt, so the benchmark # can download and decompress whatever AllPrintings format the default URL selects. find_package(ZLIB) @@ -47,7 +48,6 @@ else() message(STATUS "Oracle tests: LibLZMA not found; xz download benchmark disabled") endif() -# Oracle importer benchmark tests add_executable( oracle_importer_benchmark_test ${VERSION_STRING_CPP} ../../oracle/src/oracleimporter.cpp ../../oracle/src/parsehelpers.cpp @@ -67,6 +67,3 @@ target_link_libraries( ${TEST_QT_MODULES} ${_ORACLE_BENCH_EXTRA_LIBRARIES} ) - -add_test(NAME oracle_importer_benchmark_test COMMAND oracle_importer_benchmark_test) -set_tests_properties(oracle_importer_benchmark_test PROPERTIES TIMEOUT 120) diff --git a/tests/oracle/oracle_importer_benchmark_test.cpp b/tests/oracle/oracle_importer_benchmark_test.cpp index a52d4f332..fe2d0f107 100644 --- a/tests/oracle/oracle_importer_benchmark_test.cpp +++ b/tests/oracle/oracle_importer_benchmark_test.cpp @@ -24,6 +24,7 @@ #include "../../oracle/src/zip/unzip.h" #endif #if defined(Q_OS_MACOS) +#include #include #endif @@ -45,7 +46,12 @@ static QByteArray buildSyntheticData(int numSets, int cardsPerSet) card["colors"] = QJsonArray{"W"}; card["colorIdentity"] = QJsonArray{"W"}; card["types"] = QJsonArray{"Creature"}; - card["convertedManaCost"] = "1"; + // 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"; @@ -59,10 +65,8 @@ static QByteArray buildSyntheticData(int numSets, int cardsPerSet) identifiers["scryfallId"] = QString("id-%1-%2").arg(s).arg(c); card["identifiers"] = identifiers; - QJsonObject numObj; - numObj["number"] = QString::number(c + 1); - numObj["rarity"] = "common"; - // Add per-set properties via nested object (mtgjson format for AllPrintings) + // 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"; @@ -95,7 +99,6 @@ TEST(OracleBenchmark, ImportThroughput) QByteArray data = buildSyntheticData(numSets, cardsPerSet); - NoopCardSetPriorityController controller; OracleImporter importer; // Phase 1: Parse JSON @@ -116,6 +119,16 @@ TEST(OracleBenchmark, ImportThroughput) 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); @@ -138,8 +151,6 @@ TEST(OracleBenchmark, ParseJsonThroughput) QByteArray data = buildSyntheticData(numSets, cardsPerSet); - NoopCardSetPriorityController controller; - // Run 5 iterations and report average static constexpr int iterations = 5; qint64 totalMs = 0; @@ -312,6 +323,14 @@ struct MemorySnapshot snap.peakRssKb = usage.ru_maxrss / 1024; // bytes -> kB snap.available = snap.peakRssKb >= 0; } + // getrusage has no current-RSS equivalent; task_info's resident_size + // is the closest macOS analog to Linux VmRSS. + mach_task_basic_info info = {}; + mach_msg_type_number_t count = MACH_TASK_BASIC_INFO_COUNT; + if (task_info(mach_task_self(), MACH_TASK_BASIC_INFO, reinterpret_cast(&info), &count) == + KERN_SUCCESS) { + snap.rssKb = info.resident_size / 1024; + } #endif return snap; } @@ -331,10 +350,19 @@ static void logRamPhase(const QString &phase, const MemorySnapshot &baseline, co qDebug().noquote() << QString(" %1: memory stats unavailable on this platform").arg(phase); return; } - qDebug().noquote() << QString(" %1: current RSS %2 | peak added %3 | process peak %4") + // VmHWM / ru_maxrss are monotonically non-decreasing high-water marks, so a + // peak-based delta between phases is ~0.0 MB by construction once the + // fixture build has set the process peak. The live signals are current RSS + // and the process peak; the delta is only meaningful where current RSS is + // a per-phase value (see "after releaseSetData()"). + QString rssDelta = "N/A"; + if (current.rssKb >= 0 && baseline.rssKb >= 0) { + rssDelta = formatKb(current.rssKb - baseline.rssKb); + } + qDebug().noquote() << QString(" %1: current RSS %2 | delta vs baseline %3 | process peak %4") .arg(phase) .arg(formatKb(current.rssKb)) - .arg(formatKb(current.peakRssKb - baseline.peakRssKb)) + .arg(rssDelta) .arg(formatKb(current.peakRssKb)); } @@ -396,9 +424,17 @@ TEST(OracleBenchmark, ImportRamUsage) static constexpr int numSets = 30; static constexpr int cardsPerSet = 2000; // ~60k cards, roughly AllPrintings scale + // Baseline must precede the fixture build: a high-water mark set while + // generating the synthetic JSON would otherwise mask the importer phases. + // Where memory stats are unavailable (Windows), skip before doing the + // 60k-card fixture build, which would otherwise be pure wasted work. + const MemorySnapshot baseline = MemorySnapshot::current(); + if (!baseline.available) { + GTEST_SKIP() << "Memory stats unavailable on this platform"; + } + const QByteArray data = buildSyntheticData(numSets, cardsPerSet); - const MemorySnapshot baseline = MemorySnapshot::current(); NoopCardSetPriorityController controller; OracleImporter importer; @@ -430,12 +466,23 @@ TEST(OracleBenchmark, ImportRamUsage) TEST(OracleBenchmark, ImportRamUsageAllPrintings) { - if (qEnvironmentVariableIsEmpty("COCKATRICE_ORACLE_RAM_BENCHMARK")) { + // Only "1" enables the download: unset (the default and the CI setup) and + // an explicit "0" both disable it. + bool envOk = false; + const int enabled = qEnvironmentVariableIntValue("COCKATRICE_ORACLE_RAM_BENCHMARK", &envOk); + if (!envOk || enabled == 0) { GTEST_SKIP() << "Set COCKATRICE_ORACLE_RAM_BENCHMARK=1 to download the real AllPrintings dataset for this " "RAM benchmark. Default URL: " << kDefaultAllPrintingsUrl.toDisplayString().toStdString(); } + // Baseline must precede the request so the phase covers the download + + // decompress step, including the payload materialized by readAll(). + const MemorySnapshot baseline = MemorySnapshot::current(); + if (!baseline.available) { + GTEST_SKIP() << "Memory stats unavailable on this platform"; + } + QNetworkAccessManager nam; QNetworkRequest request(kDefaultAllPrintingsUrl); request.setHeader(QNetworkRequest::UserAgentHeader, "Cockatrice Oracle RAM benchmark"); @@ -444,19 +491,31 @@ TEST(OracleBenchmark, ImportRamUsageAllPrintings) QEventLoop loop; QTimer timeoutTimer; timeoutTimer.setSingleShot(true); + bool timedOut = false; QObject::connect(reply, &QNetworkReply::finished, &loop, &QEventLoop::quit); - QObject::connect(&timeoutTimer, &QTimer::timeout, &loop, &QEventLoop::quit); + QObject::connect(&timeoutTimer, &QTimer::timeout, &loop, [&] { + timedOut = true; + reply->abort(); + }); timeoutTimer.start(10 * 60 * 1000); loop.exec(); timeoutTimer.stop(); - if (reply->error() != QNetworkReply::NoError) { + // abort() leaves reply->error() as OperationCanceledError, so a timed-out + // download takes the same GTEST_SKIP path as any other network error + // instead of reading a truncated body and failing the parse below. + if (timedOut || reply->error() != QNetworkReply::NoError) { GTEST_SKIP() << "Download failed: " << reply->errorString().toStdString(); } const QByteArray payload = reply->readAll(); reply->deleteLater(); - const MemorySnapshot baseline = MemorySnapshot::current(); + // mtgjson can answer 200 with an HTML page (mirrors the wizard's '<' check + // in pages.cpp); reject it before trying to decompress/parse. + if (payload.startsWith("<")) { + GTEST_SKIP() << "Download returned a non-JSON body (HTML page instead of data), skipping"; + } + const QByteArray setsData = decompressSetsData(payload); const MemorySnapshot afterDownload = MemorySnapshot::current(); if (setsData.isEmpty()) { diff --git a/tests/oracle/oracle_importer_test.cpp b/tests/oracle/oracle_importer_test.cpp index 80f7794e6..19f66e5e6 100644 --- a/tests/oracle/oracle_importer_test.cpp +++ b/tests/oracle/oracle_importer_test.cpp @@ -143,8 +143,10 @@ TEST_F(OracleImporterTest, SingleColorNotSorted) // Legality guard tests // ============================================================================ -TEST_F(OracleImporterTest, LegalityGuardCombinesForNewCard) +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"; @@ -157,6 +159,25 @@ TEST_F(OracleImporterTest, LegalityGuardCombinesForNewCard) 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 @@ -206,11 +227,13 @@ 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); } @@ -219,6 +242,7 @@ 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") { @@ -233,6 +257,7 @@ 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(); @@ -252,10 +277,12 @@ TEST_F(OracleImporterTest, CreateDefaultMagicFormatsRegexMatchesBasicLands) 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.size(), second.size()); - ASSERT_EQ(first.keys(), second.keys()); + ASSERT_EQ(first.value("standard").data(), second.value("standard").data()); } // ============================================================================ @@ -314,28 +341,33 @@ TEST_F(OracleImporterTest, ReadSetsFromByteArrayCapitalizesSetType) 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"] = "zzz"; + setA["code"] = "aaa"; setA["name"] = "Zeta Set"; setA["type"] = "expansion"; setA["releaseDate"] = "2024-01-01"; setA["cards"] = QJsonArray(); QJsonObject setB; - setB["code"] = "aaa"; + setB["code"] = "zzz"; setB["name"] = "Alpha Set"; setB["type"] = "expansion"; setB["releaseDate"] = "2024-01-01"; setB["cards"] = QJsonArray(); QJsonObject root; - root["data"] = QJsonObject{{"ZZZ", setA}, {"AAA", setB}}; + 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(), "AAA"); + ASSERT_EQ(sets.first().getShortName(), "ZZZ"); } // ============================================================================ @@ -383,11 +415,9 @@ TEST_F(OracleImporterTest, SplitCardColorIdentityConcatenated) auto card = importer->getCardList().value("Fire // Ice"); ASSERT_FALSE(card.isNull()); - // coloridentity should be "RU" (concatenated), not "R // U" - QString ci = card->getProperty("coloridentity"); - ASSERT_FALSE(ci.contains("//")) << "coloridentity should not contain '//', got: " << ci.toStdString(); - ASSERT_TRUE(ci.contains("R")); - ASSERT_TRUE(ci.contains("U")); + // 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)