From 2dbf3de58a6a14822af93eb6b4ca6cb66311497d Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Wed, 21 Aug 2019 14:08:47 -0700 Subject: [PATCH 1/9] Allow more image names to be used as Nitpick test reference images --- tools/nitpick/src/TestCreator.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index 693f6085d2..834c611c48 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -323,9 +323,11 @@ void TestCreator::startTestsEvaluation( QString expectedImagePartialSourceDirectory = getExpectedImagePartialSourceDirectory(currentFilename); - // Images are stored on GitHub as ExpectedImage_ddddd.png - // Extract the digits at the end of the filename (excluding the file extension) - QString expectedImageFilenameTail = currentFilename.left(currentFilename.length() - 4).right(NUM_DIGITS); + // Images are stored on GitHub as ExpectedImage_ddddd.png or ExpectedImage_some_metadata_ddddd.png + // Extract the part of the filename after "ExpectedImage_" and excluding the file extension + QString expectedImageFilenameTail = currentFilename.left(currentFilename.lastIndexOf(".")); + int expectedImageStart = expectedImageFilenameTail.lastIndexOf(".") + 1; + expectedImageFilenameTail.remove(0, expectedImageStart); QString expectedImageStoredFilename = EXPECTED_IMAGE_PREFIX + expectedImageFilenameTail + ".png"; QString imageURLString("https://raw.githubusercontent.com/" + user + "/" + GIT_HUB_REPOSITORY + "/" + branch + "/" + From acd07e904f0389b0debb4ae68b938cb22d096087 Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Fri, 23 Aug 2019 15:50:45 -0700 Subject: [PATCH 2/9] Output profile-specific md files corresponding to profile images --- tools/nitpick/CMakeLists.txt | 2 +- tools/nitpick/src/TestCreator.cpp | 325 +++++++++++++++++++++++++----- tools/nitpick/src/TestCreator.h | 51 +++++ 3 files changed, 324 insertions(+), 54 deletions(-) diff --git a/tools/nitpick/CMakeLists.txt b/tools/nitpick/CMakeLists.txt index ee534ca24f..a5dd8d5236 100644 --- a/tools/nitpick/CMakeLists.txt +++ b/tools/nitpick/CMakeLists.txt @@ -93,7 +93,7 @@ if (WIN32) set_property(TARGET ${TARGET_NAME} APPEND_STRING PROPERTY LINK_FLAGS_DEBUG "/OPT:NOREF /OPT:NOICF") endif() -link_hifi_libraries(entities-renderer) +link_hifi_libraries(entities-renderer platform) # perform standard include and linking for found externals foreach(EXTERNAL ${OPTIONAL_EXTERNALS}) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index 834c611c48..cdca5e256a 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -15,6 +15,8 @@ #include #include #include +#include +#include #include #include @@ -497,6 +499,122 @@ void TestCreator::createTests(const QString& clientProfile) { QMessageBox::information(0, "Success", "Test images have been created"); } +void ExtractedText::error(const QString& fileName, const QString& error) { + QMessageBox::critical(0, + "Test File Parse Error", + error + " Test file: '" + fileName + "'" + ); + hasError = true; +} + +const std::vector TestProfile::tiers = [](){ + std::vector toReturn; + for (int tier = (int)platform::Profiler::Tier::LOW; tier < (int)platform::Profiler::Tier::NumTiers; ++tier) { + QString tierStringUpper = platform::Profiler::TierNames[tier]; + toReturn.push_back(tierStringUpper.toLower()); + } + return toReturn; +}(); +const std::vector TestProfile::operatingSystems = { "windows", "mac", "linux", "android" }; +const std::vector TestProfile::gpus = { "amd", "nvidia", "intel" }; + +enum class ProfileCategory { + TIER, + OS, + GPU +}; +const std::map propertyToProfileCategory = [](){ + std::map toReturn; + for (const auto& tier : TestProfile::tiers) { + toReturn[tier] = ProfileCategory::TIER; + } + for (const auto& os : TestProfile::operatingSystems) { + toReturn[os] = ProfileCategory::OS; + } + for (const auto& gpu : TestProfile::gpus) { + toReturn[gpu] = ProfileCategory::GPU; + } + return toReturn; +}(); + +std::vector TestProfile::getAllTestProfiles() { + std::vector testProfiles; + for (int tier = (int)platform::Profiler::Tier::LOW; tier < (int)platform::Profiler::Tier::NumTiers; ++tier) { + for (const auto& os : operatingSystems) { + for (const auto& gpu : gpus) { + testProfiles.push_back(TestProfile((platform::Profiler::Tier)tier, os, gpu)); + } + } + } + return testProfiles; +} + +TestFilter::TestFilter(const QString& filterString) { + auto filterParts = filterString.split(".", QString::SkipEmptyParts); + for (const auto& filterPart : filterParts) { + QList allowedVariants = filterPart.split(",", QString::SkipEmptyParts); + if (allowedVariants.empty()) { + continue; + } + + auto& referenceVariant = allowedVariants[0]; + auto foundCategoryIt = propertyToProfileCategory.find(referenceVariant); + if (foundCategoryIt == propertyToProfileCategory.cend()) { + error = "Invalid test filter property '" + referenceVariant + "'"; + return; + } else { + ProfileCategory selectedFilterCategory = foundCategoryIt->second; + for (auto allowedVariantIt = ++(allowedVariants.cbegin()); allowedVariantIt != allowedVariants.cend(); ++allowedVariantIt) { + auto& currentVariant = *allowedVariantIt; + auto nextCategoryIt = propertyToProfileCategory.find(currentVariant); + if (nextCategoryIt == propertyToProfileCategory.cend()) { + error = "Invalid test filter property '" + referenceVariant + "'"; + return; + } + auto& currentCategory = nextCategoryIt->second; + if (currentCategory != selectedFilterCategory) { + error = "Mismatched comma-separated test filter properties '" + referenceVariant + "' and '" + currentVariant + "'"; + return; + } + // List of comma-separated test property variants is consistent so far + } + + switch (selectedFilterCategory) { + case ProfileCategory::TIER: + allowedTiers.insert(allowedTiers.cend(), allowedVariants.cbegin(), allowedVariants.cend()); + break; + case ProfileCategory::OS: + allowedOperatingSystems.insert(allowedOperatingSystems.cend(), allowedVariants.cbegin(), allowedVariants.cend()); + break; + case ProfileCategory::GPU: + allowedGPUs.insert(allowedGPUs.cend(), allowedVariants.cbegin(), allowedVariants.cend()); + break; + } + } + } +} + +bool TestFilter::matches(const TestProfile& testProfile) const { + return isValid() && + (allowedTiers.empty() || std::find(allowedTiers.cbegin(), allowedTiers.cend(), testProfile.tier) != allowedTiers.cend()) && + (allowedOperatingSystems.empty() || std::find(allowedOperatingSystems.cbegin(), allowedOperatingSystems.cend(), testProfile.os) != allowedOperatingSystems.cend()) && + (allowedGPUs.empty() || std::find(allowedGPUs.cbegin(), allowedGPUs.cend(), testProfile.gpu) != allowedGPUs.cend()); +} + +bool TestFilter::isValid() const { + return error.isEmpty(); +} + +QString TestFilter::getError() const { + return error; +} + +enum class ParseTarget { + PERFORM_FUNCTION = 0, + TEST_FILTER, + NITPICK_COMMANDS +}; + ExtractedText TestCreator::getTestScriptLines(QString testFileName) { ExtractedText relevantTextFromTest; @@ -509,17 +627,20 @@ ExtractedText TestCreator::getTestScriptLines(QString testFileName) { ); } - QTextStream stream(&inputFile); - QString line = stream.readLine(); - // Name of test is the string in the following line: - // nitpick.perform("Apply Material Entities to Avatars", Script.resolvePath("."), function(testType) {... - const QString ws("\\h*"); //white-space character + // nitpick.perform("Apply Material Entities to Avatars", Script.resolvePath("."), "secondary", undefined, function(testType) {... + const QString ws("\\h*"); // One or more white-space characters const QString functionPerformName(ws + "nitpick" + ws + "\\." + ws + "perform"); - const QString quotedString("\\\".+\\\""); + const QString quote("[\"\']"); + const QString notAQuote("[^\"\']"); + const QString quotedString(quote + notAQuote + "*" + quote); QString regexTestTitle(ws + functionPerformName + "\\(" + quotedString); QRegularExpression lineContainingTitle = QRegularExpression(regexTestTitle); + // Test filter, for example: undefined, [["high"]], [["linux,mac,windows", "tier.gpu"]], etc... + // This is currently the only Nitpick property that can be set to undefined + const QRegularExpression testFilter = QRegularExpression("(undefined|(\\[[\\[\"'\\w\\h\\.,\\]]*\\])*)" + ws + ","); + // Each step is either of the following forms: // nitpick.addStepSnapshot("Take snapshot"... @@ -532,29 +653,86 @@ ExtractedText TestCreator::getTestScriptLines(QString testFileName) { const QString regexStep(ws + functionAddStepName + ws + "\\(" + ws + quotedString + ".*"); const QRegularExpression lineStep = QRegularExpression(regexStep); - while (!line.isNull()) { - line = stream.readLine(); - if (lineContainingTitle.match(line).hasMatch()) { - QStringList tokens = line.split('"'); - relevantTextFromTest.title = tokens[1]; - } else if (lineStepSnapshot.match(line).hasMatch()) { - QStringList tokens = line.split('"'); - QString nameOfStep = tokens[1]; + QTextStream stream(&inputFile); + int lineNumber = 1; + ParseTarget parseTarget = ParseTarget::PERFORM_FUNCTION; + for (QString line = stream.readLine(); !line.isNull(); line = stream.readLine()) { + switch (parseTarget) { + case ParseTarget::PERFORM_FUNCTION: + if (lineContainingTitle.match(line).hasMatch()) { + QStringList tokens = line.split('"'); + relevantTextFromTest.title = tokens[1]; + parseTarget = ParseTarget::TEST_FILTER; + } else { + break; + } + case ParseTarget::TEST_FILTER: + { + auto result = testFilter.match(line); + if (result.hasMatch()) { + // Therefore, must be in the form of a list, ideally a double list [[...]] + QString filterListDefinition = result.captured(1); + if (filterListDefinition != "undefined") { + std::string filterListDefinitionStd = filterListDefinition.toStdString(); + QJsonDocument filterListJSON = QJsonDocument::fromRawData(filterListDefinitionStd.c_str(), (int)filterListDefinitionStd.size()); + QJsonArray filterList = filterListJSON.array(); - Step *step = new Step(); - step->text = nameOfStep; - step->takeSnapshot = true; - relevantTextFromTest.stepList.emplace_back(step); + std::vector testFilters; + testFilters.reserve((size_t)filterList.size()); + for (const auto& filter : filterList) { + QJsonArray filterArgs = filter.toArray(); + if (filterArgs.isEmpty()) { + relevantTextFromTest.error(testFileName, "Invalid empty list of test filters at line " + QString::number(lineNumber) + "."); + return relevantTextFromTest; + } - } else if (lineStep.match(line).hasMatch()) { - QStringList tokens = line.split('"'); - QString nameOfStep = tokens[1]; + auto filterString = filterArgs[0].toString(); + TestFilter testFilter(filterString); + if (!testFilter.isValid()) { + relevantTextFromTest.error(testFileName, testFilter.getError() + " at line " + QString::number(lineNumber) + "."); + return relevantTextFromTest; + } else { + testFilters.push_back(testFilter); + } + } - Step *step = new Step(); - step->text = nameOfStep; - step->takeSnapshot = false; - relevantTextFromTest.stepList.emplace_back(step); + std::vector allowedVariants; + for (const auto& variant : TestProfile::getAllTestProfiles()) { + for (const auto& filter : testFilters) { + if (filter.matches(variant)) { + allowedVariants.push_back(variant); + } + } + } + relevantTextFromTest.expectedImageProfileVariants = allowedVariants; + + parseTarget = ParseTarget::NITPICK_COMMANDS; + } + } + } + break; + case ParseTarget::NITPICK_COMMANDS: + if (lineStepSnapshot.match(line).hasMatch()) { + QStringList tokens = line.split('"'); + QString nameOfStep = tokens[1]; + + Step *step = new Step(); + step->text = nameOfStep; + step->takeSnapshot = true; + relevantTextFromTest.stepList.emplace_back(step); + } else if (lineStep.match(line).hasMatch()) { + QStringList tokens = line.split('"'); + QString nameOfStep = tokens[1]; + + Step *step = new Step(); + step->text = nameOfStep; + step->takeSnapshot = false; + relevantTextFromTest.stepList.emplace_back(step); + } + break; } + + ++lineNumber; } inputFile.close(); @@ -646,6 +824,18 @@ void TestCreator::createAllMDFiles() { QMessageBox::information(0, "Success", "MD files have been created"); } +QString joinVector(const std::vector& qStringVector, char* separator) { + if (qStringVector.empty()) { + return QString(""); + } + QString joined = qStringVector[0]; + for (std::size_t i = 1; i < qStringVector.size(); ++i) { + joined += separator + qStringVector[1]; + } + return joined; +} + +// TODO: Rename this function + related functions, testScriptLines bool TestCreator::createMDFile(const QString& directory) { // Verify folder contains test.js file QString testFileName(directory + "/" + TEST_FILENAME); @@ -656,39 +846,68 @@ bool TestCreator::createMDFile(const QString& directory) { } ExtractedText testScriptLines = getTestScriptLines(testFileName); - - QString mdFilename(directory + "/" + "test.md"); - QFile mdFile(mdFilename); - if (!mdFile.open(QIODevice::WriteOnly)) { - QMessageBox::critical(0, "Internal error: " + QString(__FILE__) + ":" + QString::number(__LINE__), "Failed to create file " + mdFilename); - exit(-1); + if (testScriptLines.hasError) { + return false; } - QTextStream stream(&mdFile); + QDir qDirectory(directory); - //TestCreator title - QString testName = testScriptLines.title; - stream << "# " << testName << "\n"; - - stream << "## Run this script URL: [Manual](./test.js?raw=true) [Auto](./testAuto.js?raw=true)(from menu/Edit/Open and Run scripts from URL...)." << "\n\n"; - - stream << "## Preconditions" << "\n"; - stream << "- In an empty region of a domain with editing rights." << "\n\n"; - - stream << "## Steps\n"; - stream << "Press '" + ADVANCE_KEY + "' key to advance step by step\n\n"; // note apostrophes surrounding 'ADVANCE_KEY' - - int snapShotIndex { 0 }; - for (size_t i = 0; i < testScriptLines.stepList.size(); ++i) { - stream << "### Step " << QString::number(i + 1) << "\n"; - stream << "- " << testScriptLines.stepList[i]->text << "\n"; - if ((i + 1 < testScriptLines.stepList.size()) && testScriptLines.stepList[i]->takeSnapshot) { - stream << "- ![](./ExpectedImage_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png)\n"; - ++snapShotIndex; + // ExpectedImage_00000.png OR ExpectedImage_some_stu-ff_00000.png + const QRegularExpression firstExpectedImage("^ExpectedImage(_[-_\\w]*)?_00000\\.png$"); + for (const auto& potentialImageFile : qDirectory.entryInfoList()) { + if (potentialImageFile.isDir()) { + continue; } - } - mdFile.close(); + auto firstExpectedImageMatch = firstExpectedImage.match(potentialImageFile.fileName()); + if (!firstExpectedImageMatch.hasMatch()) { + continue; + } + + QString testDescriptor = firstExpectedImageMatch.captured(1); + auto filterString = QString(testDescriptor).replace("_", ".").replace("-", ","); + TestFilter descriptorAsFilter(filterString); + + QString mdFilename(directory + "/" + "test" + testDescriptor + ".md"); + QFile mdFile(mdFilename); + if (!mdFile.open(QIODevice::WriteOnly)) { + QMessageBox::critical(0, "Internal error: " + QString(__FILE__) + ":" + QString::number(__LINE__), "Failed to create file " + mdFilename); + // TODO: Don't just exit + exit(-1); + } + + QTextStream stream(&mdFile); + + QString testName = testScriptLines.title; + stream << "# " << testName << "\n"; + + stream << "## Run this script URL: [Manual](./test.js?raw=true) [Auto](./testAuto.js?raw=true)(from menu/Edit/Open and Run scripts from URL...)." << "\n\n"; + + stream << "## Preconditions" << "\n"; + stream << "- In an empty region of a domain with editing rights." << "\n"; + stream << "- Tier: " << (descriptorAsFilter.allowedTiers.empty() ? "any" : joinVector(descriptorAsFilter.allowedTiers, ", ")) << "\n"; + stream << "- OS: " << (descriptorAsFilter.allowedOperatingSystems.empty() ? "any" : joinVector(descriptorAsFilter.allowedOperatingSystems, ", ")) << "\n"; + stream << "- GPU: " << (descriptorAsFilter.allowedGPUs.empty() ? "any" : joinVector(descriptorAsFilter.allowedGPUs, ", ")) << "\n"; + if (!descriptorAsFilter.isValid()) { + stream << "\nWarning: The profile preconditions were unsuccessfully read from the expected image name and may be incorrect. Error message: " << descriptorAsFilter.getError() << "\n"; + } + stream << "\n"; + + stream << "## Steps\n"; + stream << "Press '" + ADVANCE_KEY + "' key to advance step by step\n\n"; // note apostrophes surrounding 'ADVANCE_KEY' + + int snapShotIndex { 0 }; + for (size_t i = 0; i < testScriptLines.stepList.size(); ++i) { + stream << "### Step " << QString::number(i + 1) << "\n"; + stream << "- " << testScriptLines.stepList[i]->text << "\n"; + if ((i + 1 < testScriptLines.stepList.size()) && testScriptLines.stepList[i]->takeSnapshot) { + stream << "- ![](./ExpectedImage" << testDescriptor << "_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png)\n"; + ++snapShotIndex; + } + } + + mdFile.close(); + } foreach (auto test, testScriptLines.stepList) { delete test; diff --git a/tools/nitpick/src/TestCreator.h b/tools/nitpick/src/TestCreator.h index f995c06c94..f7e4af816a 100644 --- a/tools/nitpick/src/TestCreator.h +++ b/tools/nitpick/src/TestCreator.h @@ -16,6 +16,8 @@ #include #include +#include + #include "AWSInterface.h" #include "ImageComparer.h" #include "Downloader.h" @@ -30,10 +32,59 @@ public: using StepList = std::vector; +class TestProfile { +public: + TestProfile() {} + TestProfile(platform::Profiler::Tier tier, const QString& os, const QString& gpu) : + tier(tier), + os(os), + gpu(gpu) { + } + TestProfile(const TestProfile& other) : + tier(other.tier), + os(other.os), + gpu(other.gpu) { + } + bool operator==(const TestProfile& other) const { + return tier == other.tier && + os == other.os && + gpu == other.gpu; + } + + const static std::vector tiers; + const static std::vector operatingSystems; + const static std::vector gpus; + + static std::vector getAllTestProfiles(); + + QString tier; + QString os; + QString gpu; +}; + +class TestFilter { +public: + TestFilter(const QString& filterString); + bool matches(const TestProfile& testProfile) const; + bool isValid() const; + QString getError() const; + + std::vector allowedTiers; + std::vector allowedOperatingSystems; + std::vector allowedGPUs; + +protected: + QString error; +}; + class ExtractedText { public: QString title; StepList stepList; + std::vector expectedImageProfileVariants; + + bool hasError { false }; + void error(const QString& fileName, const QString& error); }; enum TestRailCreateMode { From 5e3777999dcf5cda0f774a1c63ba739a7a7bd64c Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 09:57:48 -0700 Subject: [PATCH 3/9] Revert unfinished alternative implementation --- tools/nitpick/src/TestCreator.cpp | 169 +++++++----------------------- tools/nitpick/src/TestCreator.h | 35 ------- 2 files changed, 37 insertions(+), 167 deletions(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index cdca5e256a..c4e778cfc4 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -15,8 +15,6 @@ #include #include #include -#include -#include #include #include @@ -499,24 +497,20 @@ void TestCreator::createTests(const QString& clientProfile) { QMessageBox::information(0, "Success", "Test images have been created"); } -void ExtractedText::error(const QString& fileName, const QString& error) { - QMessageBox::critical(0, - "Test File Parse Error", - error + " Test file: '" + fileName + "'" - ); - hasError = true; -} +namespace TestProfile { + std::vector tiers = [](){ + std::vector toReturn; + for (int tier = (int)platform::Profiler::Tier::LOW; tier < (int)platform::Profiler::Tier::NumTiers; ++tier) { + QString tierStringUpper = platform::Profiler::TierNames[tier]; + toReturn.push_back(tierStringUpper.toLower()); + } + return toReturn; + }(); -const std::vector TestProfile::tiers = [](){ - std::vector toReturn; - for (int tier = (int)platform::Profiler::Tier::LOW; tier < (int)platform::Profiler::Tier::NumTiers; ++tier) { - QString tierStringUpper = platform::Profiler::TierNames[tier]; - toReturn.push_back(tierStringUpper.toLower()); - } - return toReturn; -}(); -const std::vector TestProfile::operatingSystems = { "windows", "mac", "linux", "android" }; -const std::vector TestProfile::gpus = { "amd", "nvidia", "intel" }; + std::vector operatingSystems = { "windows", "mac", "linux", "android" }; + + std::vector gpus = { "amd", "nvidia", "intel" }; +}; enum class ProfileCategory { TIER, @@ -537,18 +531,6 @@ const std::map propertyToProfileCategory = [](){ return toReturn; }(); -std::vector TestProfile::getAllTestProfiles() { - std::vector testProfiles; - for (int tier = (int)platform::Profiler::Tier::LOW; tier < (int)platform::Profiler::Tier::NumTiers; ++tier) { - for (const auto& os : operatingSystems) { - for (const auto& gpu : gpus) { - testProfiles.push_back(TestProfile((platform::Profiler::Tier)tier, os, gpu)); - } - } - } - return testProfiles; -} - TestFilter::TestFilter(const QString& filterString) { auto filterParts = filterString.split(".", QString::SkipEmptyParts); for (const auto& filterPart : filterParts) { @@ -594,13 +576,6 @@ TestFilter::TestFilter(const QString& filterString) { } } -bool TestFilter::matches(const TestProfile& testProfile) const { - return isValid() && - (allowedTiers.empty() || std::find(allowedTiers.cbegin(), allowedTiers.cend(), testProfile.tier) != allowedTiers.cend()) && - (allowedOperatingSystems.empty() || std::find(allowedOperatingSystems.cbegin(), allowedOperatingSystems.cend(), testProfile.os) != allowedOperatingSystems.cend()) && - (allowedGPUs.empty() || std::find(allowedGPUs.cbegin(), allowedGPUs.cend(), testProfile.gpu) != allowedGPUs.cend()); -} - bool TestFilter::isValid() const { return error.isEmpty(); } @@ -609,12 +584,6 @@ QString TestFilter::getError() const { return error; } -enum class ParseTarget { - PERFORM_FUNCTION = 0, - TEST_FILTER, - NITPICK_COMMANDS -}; - ExtractedText TestCreator::getTestScriptLines(QString testFileName) { ExtractedText relevantTextFromTest; @@ -627,20 +596,17 @@ ExtractedText TestCreator::getTestScriptLines(QString testFileName) { ); } + QTextStream stream(&inputFile); + QString line = stream.readLine(); + // Name of test is the string in the following line: // nitpick.perform("Apply Material Entities to Avatars", Script.resolvePath("."), "secondary", undefined, function(testType) {... - const QString ws("\\h*"); // One or more white-space characters + const QString ws("\\h*"); //white-space character const QString functionPerformName(ws + "nitpick" + ws + "\\." + ws + "perform"); - const QString quote("[\"\']"); - const QString notAQuote("[^\"\']"); - const QString quotedString(quote + notAQuote + "*" + quote); + const QString quotedString("\\\".+\\\""); QString regexTestTitle(ws + functionPerformName + "\\(" + quotedString); QRegularExpression lineContainingTitle = QRegularExpression(regexTestTitle); - // Test filter, for example: undefined, [["high"]], [["linux,mac,windows", "tier.gpu"]], etc... - // This is currently the only Nitpick property that can be set to undefined - const QRegularExpression testFilter = QRegularExpression("(undefined|(\\[[\\[\"'\\w\\h\\.,\\]]*\\])*)" + ws + ","); - // Each step is either of the following forms: // nitpick.addStepSnapshot("Take snapshot"... @@ -653,86 +619,29 @@ ExtractedText TestCreator::getTestScriptLines(QString testFileName) { const QString regexStep(ws + functionAddStepName + ws + "\\(" + ws + quotedString + ".*"); const QRegularExpression lineStep = QRegularExpression(regexStep); - QTextStream stream(&inputFile); - int lineNumber = 1; - ParseTarget parseTarget = ParseTarget::PERFORM_FUNCTION; - for (QString line = stream.readLine(); !line.isNull(); line = stream.readLine()) { - switch (parseTarget) { - case ParseTarget::PERFORM_FUNCTION: - if (lineContainingTitle.match(line).hasMatch()) { - QStringList tokens = line.split('"'); - relevantTextFromTest.title = tokens[1]; - parseTarget = ParseTarget::TEST_FILTER; - } else { - break; - } - case ParseTarget::TEST_FILTER: - { - auto result = testFilter.match(line); - if (result.hasMatch()) { - // Therefore, must be in the form of a list, ideally a double list [[...]] - QString filterListDefinition = result.captured(1); - if (filterListDefinition != "undefined") { - std::string filterListDefinitionStd = filterListDefinition.toStdString(); - QJsonDocument filterListJSON = QJsonDocument::fromRawData(filterListDefinitionStd.c_str(), (int)filterListDefinitionStd.size()); - QJsonArray filterList = filterListJSON.array(); + while (!line.isNull()) { + line = stream.readLine(); + if (lineContainingTitle.match(line).hasMatch()) { + QStringList tokens = line.split('"'); + relevantTextFromTest.title = tokens[1]; + } else if (lineStepSnapshot.match(line).hasMatch()) { + QStringList tokens = line.split('"'); + QString nameOfStep = tokens[1]; - std::vector testFilters; - testFilters.reserve((size_t)filterList.size()); - for (const auto& filter : filterList) { - QJsonArray filterArgs = filter.toArray(); - if (filterArgs.isEmpty()) { - relevantTextFromTest.error(testFileName, "Invalid empty list of test filters at line " + QString::number(lineNumber) + "."); - return relevantTextFromTest; - } + Step *step = new Step(); + step->text = nameOfStep; + step->takeSnapshot = true; + relevantTextFromTest.stepList.emplace_back(step); - auto filterString = filterArgs[0].toString(); - TestFilter testFilter(filterString); - if (!testFilter.isValid()) { - relevantTextFromTest.error(testFileName, testFilter.getError() + " at line " + QString::number(lineNumber) + "."); - return relevantTextFromTest; - } else { - testFilters.push_back(testFilter); - } - } + } else if (lineStep.match(line).hasMatch()) { + QStringList tokens = line.split('"'); + QString nameOfStep = tokens[1]; - std::vector allowedVariants; - for (const auto& variant : TestProfile::getAllTestProfiles()) { - for (const auto& filter : testFilters) { - if (filter.matches(variant)) { - allowedVariants.push_back(variant); - } - } - } - relevantTextFromTest.expectedImageProfileVariants = allowedVariants; - - parseTarget = ParseTarget::NITPICK_COMMANDS; - } - } - } - break; - case ParseTarget::NITPICK_COMMANDS: - if (lineStepSnapshot.match(line).hasMatch()) { - QStringList tokens = line.split('"'); - QString nameOfStep = tokens[1]; - - Step *step = new Step(); - step->text = nameOfStep; - step->takeSnapshot = true; - relevantTextFromTest.stepList.emplace_back(step); - } else if (lineStep.match(line).hasMatch()) { - QStringList tokens = line.split('"'); - QString nameOfStep = tokens[1]; - - Step *step = new Step(); - step->text = nameOfStep; - step->takeSnapshot = false; - relevantTextFromTest.stepList.emplace_back(step); - } - break; + Step *step = new Step(); + step->text = nameOfStep; + step->takeSnapshot = false; + relevantTextFromTest.stepList.emplace_back(step); } - - ++lineNumber; } inputFile.close(); @@ -835,7 +744,6 @@ QString joinVector(const std::vector& qStringVector, char* separator) { return joined; } -// TODO: Rename this function + related functions, testScriptLines bool TestCreator::createMDFile(const QString& directory) { // Verify folder contains test.js file QString testFileName(directory + "/" + TEST_FILENAME); @@ -846,9 +754,6 @@ bool TestCreator::createMDFile(const QString& directory) { } ExtractedText testScriptLines = getTestScriptLines(testFileName); - if (testScriptLines.hasError) { - return false; - } QDir qDirectory(directory); diff --git a/tools/nitpick/src/TestCreator.h b/tools/nitpick/src/TestCreator.h index f7e4af816a..247f2ee117 100644 --- a/tools/nitpick/src/TestCreator.h +++ b/tools/nitpick/src/TestCreator.h @@ -32,40 +32,9 @@ public: using StepList = std::vector; -class TestProfile { -public: - TestProfile() {} - TestProfile(platform::Profiler::Tier tier, const QString& os, const QString& gpu) : - tier(tier), - os(os), - gpu(gpu) { - } - TestProfile(const TestProfile& other) : - tier(other.tier), - os(other.os), - gpu(other.gpu) { - } - bool operator==(const TestProfile& other) const { - return tier == other.tier && - os == other.os && - gpu == other.gpu; - } - - const static std::vector tiers; - const static std::vector operatingSystems; - const static std::vector gpus; - - static std::vector getAllTestProfiles(); - - QString tier; - QString os; - QString gpu; -}; - class TestFilter { public: TestFilter(const QString& filterString); - bool matches(const TestProfile& testProfile) const; bool isValid() const; QString getError() const; @@ -81,10 +50,6 @@ class ExtractedText { public: QString title; StepList stepList; - std::vector expectedImageProfileVariants; - - bool hasError { false }; - void error(const QString& fileName, const QString& error); }; enum TestRailCreateMode { From 1ae66c6c104b8dc3049a443e482a2c5066c2daf0 Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 10:49:16 -0700 Subject: [PATCH 4/9] Show expected images side-by-side rather than creating one MD file per image set --- tools/nitpick/src/TestCreator.cpp | 98 ++++++++++++++++--------------- 1 file changed, 51 insertions(+), 47 deletions(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index c4e778cfc4..dee8f7d65d 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -757,63 +757,67 @@ bool TestCreator::createMDFile(const QString& directory) { QDir qDirectory(directory); + QString mdFilename(directory + "/" + "test.md"); + QFile mdFile(mdFilename); + if (!mdFile.open(QIODevice::WriteOnly)) { + QMessageBox::critical(0, "Internal error: " + QString(__FILE__) + ":" + QString::number(__LINE__), "Failed to create file " + mdFilename); + // TODO: Don't just exit + exit(-1); + } + + QTextStream stream(&mdFile); + + QString testName = testScriptLines.title; + stream << "# " << testName << "\n"; + + stream << "## Run this script URL: [Manual](./test.js?raw=true) [Auto](./testAuto.js?raw=true)(from menu/Edit/Open and Run scripts from URL...)." << "\n\n"; + + stream << "## Preconditions" << "\n"; + stream << "- In an empty region of a domain with editing rights." << "\n"; + stream << "\n"; + // ExpectedImage_00000.png OR ExpectedImage_some_stu-ff_00000.png const QRegularExpression firstExpectedImage("^ExpectedImage(_[-_\\w]*)?_00000\\.png$"); - for (const auto& potentialImageFile : qDirectory.entryInfoList()) { - if (potentialImageFile.isDir()) { - continue; - } - auto firstExpectedImageMatch = firstExpectedImage.match(potentialImageFile.fileName()); - if (!firstExpectedImageMatch.hasMatch()) { - continue; - } + stream << "## Steps\n"; + stream << "Press '" + ADVANCE_KEY + "' key to advance step by step\n\n"; // note apostrophes surrounding 'ADVANCE_KEY' + int snapShotIndex { 0 }; + for (size_t i = 0; i < testScriptLines.stepList.size(); ++i) { + stream << "### Step " << QString::number(i + 1) << "\n"; + stream << "- " << testScriptLines.stepList[i]->text << "\n"; + if ((i + 1 < testScriptLines.stepList.size()) && testScriptLines.stepList[i]->takeSnapshot) { + for (const auto& potentialImageFile : qDirectory.entryInfoList()) { + if (potentialImageFile.isDir()) { + continue; + } - QString testDescriptor = firstExpectedImageMatch.captured(1); - auto filterString = QString(testDescriptor).replace("_", ".").replace("-", ","); - TestFilter descriptorAsFilter(filterString); + auto firstExpectedImageMatch = firstExpectedImage.match(potentialImageFile.fileName()); + if (!firstExpectedImageMatch.hasMatch()) { + continue; + } - QString mdFilename(directory + "/" + "test" + testDescriptor + ".md"); - QFile mdFile(mdFilename); - if (!mdFile.open(QIODevice::WriteOnly)) { - QMessageBox::critical(0, "Internal error: " + QString(__FILE__) + ":" + QString::number(__LINE__), "Failed to create file " + mdFilename); - // TODO: Don't just exit - exit(-1); - } - - QTextStream stream(&mdFile); - - QString testName = testScriptLines.title; - stream << "# " << testName << "\n"; - - stream << "## Run this script URL: [Manual](./test.js?raw=true) [Auto](./testAuto.js?raw=true)(from menu/Edit/Open and Run scripts from URL...)." << "\n\n"; - - stream << "## Preconditions" << "\n"; - stream << "- In an empty region of a domain with editing rights." << "\n"; - stream << "- Tier: " << (descriptorAsFilter.allowedTiers.empty() ? "any" : joinVector(descriptorAsFilter.allowedTiers, ", ")) << "\n"; - stream << "- OS: " << (descriptorAsFilter.allowedOperatingSystems.empty() ? "any" : joinVector(descriptorAsFilter.allowedOperatingSystems, ", ")) << "\n"; - stream << "- GPU: " << (descriptorAsFilter.allowedGPUs.empty() ? "any" : joinVector(descriptorAsFilter.allowedGPUs, ", ")) << "\n"; - if (!descriptorAsFilter.isValid()) { - stream << "\nWarning: The profile preconditions were unsuccessfully read from the expected image name and may be incorrect. Error message: " << descriptorAsFilter.getError() << "\n"; - } - stream << "\n"; - - stream << "## Steps\n"; - stream << "Press '" + ADVANCE_KEY + "' key to advance step by step\n\n"; // note apostrophes surrounding 'ADVANCE_KEY' - - int snapShotIndex { 0 }; - for (size_t i = 0; i < testScriptLines.stepList.size(); ++i) { - stream << "### Step " << QString::number(i + 1) << "\n"; - stream << "- " << testScriptLines.stepList[i]->text << "\n"; - if ((i + 1 < testScriptLines.stepList.size()) && testScriptLines.stepList[i]->takeSnapshot) { + QString testDescriptor = firstExpectedImageMatch.captured(1); + auto filterString = QString(testDescriptor).replace("_", ".").replace("-", ","); + TestFilter descriptorAsFilter(filterString); + if (descriptorAsFilter.isValid()) { + stream << "- Expected image on "; + stream << (descriptorAsFilter.allowedTiers.empty() ? "any" : joinVector(descriptorAsFilter.allowedTiers, "/")) << " tier, "; + stream << (descriptorAsFilter.allowedOperatingSystems.empty() ? "any" : joinVector(descriptorAsFilter.allowedOperatingSystems, "/")) << " OS, "; + stream << (descriptorAsFilter.allowedGPUs.empty() ? "any" : joinVector(descriptorAsFilter.allowedGPUs, "/")) << " GPU"; + stream << ":"; + } else { + // Fall back to displaying file name + stream << "ExpectedImage" << testDescriptor << "_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png"; + } + stream << "\n"; stream << "- ![](./ExpectedImage" << testDescriptor << "_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png)\n"; - ++snapShotIndex; } + ++snapShotIndex; } - - mdFile.close(); } + mdFile.close(); + foreach (auto test, testScriptLines.stepList) { delete test; } From e4369feec5043c478baa47f8c48a43460cc0e74f Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 11:33:03 -0700 Subject: [PATCH 5/9] Factor out directory parsing from md file output --- tools/nitpick/src/TestCreator.cpp | 36 ++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index dee8f7d65d..709331c45e 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -778,6 +778,26 @@ bool TestCreator::createMDFile(const QString& directory) { // ExpectedImage_00000.png OR ExpectedImage_some_stu-ff_00000.png const QRegularExpression firstExpectedImage("^ExpectedImage(_[-_\\w]*)?_00000\\.png$"); + std::vector testDescriptors; + std::vector testFilters; + + for (const auto& potentialImageFile : qDirectory.entryInfoList()) { + if (potentialImageFile.isDir()) { + continue; + } + + auto firstExpectedImageMatch = firstExpectedImage.match(potentialImageFile.fileName()); + if (!firstExpectedImageMatch.hasMatch()) { + continue; + } + + QString testDescriptor = firstExpectedImageMatch.captured(1); + auto filterString = QString(testDescriptor).replace("_", ".").replace("-", ","); + TestFilter descriptorAsFilter(filterString); + + testDescriptors.push_back(testDescriptor); + testFilters.push_back(descriptorAsFilter); + } stream << "## Steps\n"; stream << "Press '" + ADVANCE_KEY + "' key to advance step by step\n\n"; // note apostrophes surrounding 'ADVANCE_KEY' @@ -786,19 +806,9 @@ bool TestCreator::createMDFile(const QString& directory) { stream << "### Step " << QString::number(i + 1) << "\n"; stream << "- " << testScriptLines.stepList[i]->text << "\n"; if ((i + 1 < testScriptLines.stepList.size()) && testScriptLines.stepList[i]->takeSnapshot) { - for (const auto& potentialImageFile : qDirectory.entryInfoList()) { - if (potentialImageFile.isDir()) { - continue; - } - - auto firstExpectedImageMatch = firstExpectedImage.match(potentialImageFile.fileName()); - if (!firstExpectedImageMatch.hasMatch()) { - continue; - } - - QString testDescriptor = firstExpectedImageMatch.captured(1); - auto filterString = QString(testDescriptor).replace("_", ".").replace("-", ","); - TestFilter descriptorAsFilter(filterString); + for (int i = 0; i < testDescriptors.size(); ++i) { + const auto& testDescriptor = testDescriptors[i]; + const auto& descriptorAsFilter = testFilters[i]; if (descriptorAsFilter.isValid()) { stream << "- Expected image on "; stream << (descriptorAsFilter.allowedTiers.empty() ? "any" : joinVector(descriptorAsFilter.allowedTiers, "/")) << " tier, "; From bc2cc11c06907d0a1a5ddaa5599f3e0f125fec6d Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 12:35:18 -0700 Subject: [PATCH 6/9] Fix build warnings --- tools/nitpick/src/TestCreator.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index 709331c45e..2894e195fb 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -733,7 +733,7 @@ void TestCreator::createAllMDFiles() { QMessageBox::information(0, "Success", "MD files have been created"); } -QString joinVector(const std::vector& qStringVector, char* separator) { +QString joinVector(const std::vector& qStringVector, const char* separator) { if (qStringVector.empty()) { return QString(""); } @@ -806,7 +806,7 @@ bool TestCreator::createMDFile(const QString& directory) { stream << "### Step " << QString::number(i + 1) << "\n"; stream << "- " << testScriptLines.stepList[i]->text << "\n"; if ((i + 1 < testScriptLines.stepList.size()) && testScriptLines.stepList[i]->takeSnapshot) { - for (int i = 0; i < testDescriptors.size(); ++i) { + for (int i = 0; i < (int)testDescriptors.size(); ++i) { const auto& testDescriptor = testDescriptors[i]; const auto& descriptorAsFilter = testFilters[i]; if (descriptorAsFilter.isValid()) { From 388a70502ee53f6688c0c75dc0114bb0e8e1843d Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 14:28:04 -0700 Subject: [PATCH 7/9] Fix indexing typo in joinVector --- tools/nitpick/src/TestCreator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index 2894e195fb..412fc95390 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -739,7 +739,7 @@ QString joinVector(const std::vector& qStringVector, const char* separa } QString joined = qStringVector[0]; for (std::size_t i = 1; i < qStringVector.size(); ++i) { - joined += separator + qStringVector[1]; + joined += separator + qStringVector[i]; } return joined; } From ad1a98b85a67309f2604362e867d0a5eafb030fc Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 14:30:47 -0700 Subject: [PATCH 8/9] Trim else after return --- tools/nitpick/src/TestCreator.cpp | 52 +++++++++++++++---------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index 412fc95390..e6a3224cf7 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -544,34 +544,34 @@ TestFilter::TestFilter(const QString& filterString) { if (foundCategoryIt == propertyToProfileCategory.cend()) { error = "Invalid test filter property '" + referenceVariant + "'"; return; - } else { - ProfileCategory selectedFilterCategory = foundCategoryIt->second; - for (auto allowedVariantIt = ++(allowedVariants.cbegin()); allowedVariantIt != allowedVariants.cend(); ++allowedVariantIt) { - auto& currentVariant = *allowedVariantIt; - auto nextCategoryIt = propertyToProfileCategory.find(currentVariant); - if (nextCategoryIt == propertyToProfileCategory.cend()) { - error = "Invalid test filter property '" + referenceVariant + "'"; - return; - } - auto& currentCategory = nextCategoryIt->second; - if (currentCategory != selectedFilterCategory) { - error = "Mismatched comma-separated test filter properties '" + referenceVariant + "' and '" + currentVariant + "'"; - return; - } - // List of comma-separated test property variants is consistent so far - } + } - switch (selectedFilterCategory) { - case ProfileCategory::TIER: - allowedTiers.insert(allowedTiers.cend(), allowedVariants.cbegin(), allowedVariants.cend()); - break; - case ProfileCategory::OS: - allowedOperatingSystems.insert(allowedOperatingSystems.cend(), allowedVariants.cbegin(), allowedVariants.cend()); - break; - case ProfileCategory::GPU: - allowedGPUs.insert(allowedGPUs.cend(), allowedVariants.cbegin(), allowedVariants.cend()); - break; + ProfileCategory selectedFilterCategory = foundCategoryIt->second; + for (auto allowedVariantIt = ++(allowedVariants.cbegin()); allowedVariantIt != allowedVariants.cend(); ++allowedVariantIt) { + auto& currentVariant = *allowedVariantIt; + auto nextCategoryIt = propertyToProfileCategory.find(currentVariant); + if (nextCategoryIt == propertyToProfileCategory.cend()) { + error = "Invalid test filter property '" + referenceVariant + "'"; + return; } + auto& currentCategory = nextCategoryIt->second; + if (currentCategory != selectedFilterCategory) { + error = "Mismatched comma-separated test filter properties '" + referenceVariant + "' and '" + currentVariant + "'"; + return; + } + // List of comma-separated test property variants is consistent so far + } + + switch (selectedFilterCategory) { + case ProfileCategory::TIER: + allowedTiers.insert(allowedTiers.cend(), allowedVariants.cbegin(), allowedVariants.cend()); + break; + case ProfileCategory::OS: + allowedOperatingSystems.insert(allowedOperatingSystems.cend(), allowedVariants.cbegin(), allowedVariants.cend()); + break; + case ProfileCategory::GPU: + allowedGPUs.insert(allowedGPUs.cend(), allowedVariants.cbegin(), allowedVariants.cend()); + break; } } } From e0b47349171b604f634fa51dd8fa3253db73236b Mon Sep 17 00:00:00 2001 From: sabrina-shanman Date: Mon, 26 Aug 2019 14:37:37 -0700 Subject: [PATCH 9/9] Add missing bullet point when test.md step has image with unknown test filter --- tools/nitpick/src/TestCreator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/nitpick/src/TestCreator.cpp b/tools/nitpick/src/TestCreator.cpp index e6a3224cf7..652292b9dc 100644 --- a/tools/nitpick/src/TestCreator.cpp +++ b/tools/nitpick/src/TestCreator.cpp @@ -817,7 +817,7 @@ bool TestCreator::createMDFile(const QString& directory) { stream << ":"; } else { // Fall back to displaying file name - stream << "ExpectedImage" << testDescriptor << "_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png"; + stream << "- ExpectedImage" << testDescriptor << "_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png"; } stream << "\n"; stream << "- ![](./ExpectedImage" << testDescriptor << "_" << QString::number(snapShotIndex).rightJustified(5, '0') << ".png)\n";