AutoTest: Redo evaluation of result summary items

By moving this functionality completely into the tree item
it is possible to eliminate superfluous enum values that
basically represented the same item but in different states.

Change-Id: I2e6b6462bbaae51bdf6822fde198aea36f951c7f
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/autotest/qtest/qttestresult.cpp

@@ -84,7 +84,7 @@ bool QtTestResult::isDirectParentOf(const TestResult *other, bool *needsIntermed
         return false;
     const QtTestResult *qtOther = static_cast<const QtTestResult *>(other);
 
-    if (TestResult::isMessageCaseStart(result())) {
+    if (result() == ResultType::TestStart) {
         if (qtOther->isDataTag()) {
             if (qtOther->m_function == m_function) {
                 if (m_dataTag.isEmpty()) {

src/plugins/autotest/testresult.cpp

@@ -97,12 +97,8 @@ QString TestResult::resultToString(const ResultType type)
 {
     switch (type) {
     case ResultType::Pass:
-    case ResultType::MessageTestCaseSuccess:
-    case ResultType::MessageTestCaseSuccessWarn:
         return QString("PASS");
     case ResultType::Fail:
-    case ResultType::MessageTestCaseFail:
-    case ResultType::MessageTestCaseFailWarn:
         return QString("FAIL");
     case ResultType::ExpectedFail:
         return QString("XFAIL");
@@ -174,13 +170,6 @@ QColor TestResult::colorForType(const ResultType type)
     }
 }
 
-bool TestResult::isMessageCaseStart(const ResultType type)
-{
-    return type == ResultType::TestStart || type == ResultType::MessageTestCaseSuccess
-            || type == ResultType::MessageTestCaseFail || type == ResultType::MessageTestCaseSuccessWarn
-            || type == ResultType::MessageTestCaseFailWarn;
-}
-
 bool TestResult::isDirectParentOf(const TestResult *other, bool * /*needsIntermediate*/) const
 {
     QTC_ASSERT(other, return false);

src/plugins/autotest/testresult.h

@@ -61,12 +61,8 @@ enum class ResultType {
     MessageLocation,
     // anything below is an internal message (or a pure message without icon)
     MessageInternal, INTERNAL_MESSAGES_BEGIN = MessageInternal,
-    // TODO make the following 5 a single start item (get icon/short text depending on children)
+    // start item (get icon/short text depending on children)
     TestStart,
-    MessageTestCaseSuccess,
-    MessageTestCaseSuccessWarn,
-    MessageTestCaseFail,
-    MessageTestCaseFailWarn,
     // usually no icon/short text - more or less an indicator (and can contain test duration)
     TestEnd,
     // special global (temporary) message
@@ -108,7 +104,6 @@ public:
     static ResultType toResultType(int rt);
     static QString resultToString(const ResultType type);
     static QColor colorForType(const ResultType type);
-    static bool isMessageCaseStart(const ResultType type);
 
     virtual bool isDirectParentOf(const TestResult *other, bool *needsIntermediate) const;
     virtual bool isIntermediateFor(const TestResult *other) const;

src/plugins/autotest/testresultdelegate.cpp

@@ -83,12 +83,14 @@ void TestResultDelegate::paint(QPainter *painter, const QStyleOptionViewItem &op
         painter->drawPixmap(positions.left(), positions.top(),
                             icon.pixmap(window, QSize(positions.iconSize(), positions.iconSize())));
 
-    QString typeStr = TestResult::resultToString(testResult->result());
+    TestResultItem *item = resultFilterModel->itemForIndex(index);
+    QTC_ASSERT(item, painter->restore(); return);
+    const QString typeStr = item->resultString();
     if (selected) {
         painter->drawText(positions.typeAreaLeft(), positions.top() + fm.ascent(), typeStr);
     } else {
         QPen tmp = painter->pen();
-        if (TestResult::isMessageCaseStart(testResult->result()))
+        if (testResult->result() == ResultType::TestStart)
             painter->setPen(opt.palette.mid().color());
         else
             painter->setPen(TestResult::colorForType(testResult->result()));

src/plugins/autotest/testresultmodel.cpp

@@ -62,24 +62,13 @@ static QIcon testResultIcon(ResultType result) {
         Icons::RESULT_MESSAGEWARN.icon(),
         Icons::RESULT_MESSAGEFATAL.icon(),
         Icons::RESULT_MESSAGEFATAL.icon(), // System gets same handling as Fatal for now
-        QIcon(),
-        Icons::RESULT_MESSAGEPASSWARN.icon(),
-        Icons::RESULT_MESSAGEFAILWARN.icon(),
         ProjectExplorer::Icons::DESKTOP_DEVICE.icon(),  // for now
     }; // provide an icon for unknown??
 
     if (result < ResultType::FIRST_TYPE || result >= ResultType::MessageInternal) {
         switch (result) {
-        case ResultType::MessageTestCaseSuccess:
-            return icons[int(ResultType::Pass)];
-        case ResultType::MessageTestCaseFail:
-            return icons[int(ResultType::Fail)];
-        case ResultType::MessageTestCaseSuccessWarn:
-            return icons[16];
-        case ResultType::MessageTestCaseFailWarn:
-            return icons[17];
         case ResultType::Application:
-            return icons[18];
+            return icons[15];
         default:
             return QIcon();
         }
@@ -87,6 +76,15 @@ static QIcon testResultIcon(ResultType result) {
     return icons[int(result)];
 }
 
+static QIcon testSummaryIcon(const Utils::optional<TestResultItem::SummaryEvaluation> &summary)
+{
+    if (!summary)
+        return QIcon();
+    if (summary->failed)
+        return summary->warnings ? Icons::RESULT_MESSAGEFAILWARN.icon() : Icons::RESULT_FAIL.icon();
+    return summary->warnings ? Icons::RESULT_MESSAGEPASSWARN.icon() : Icons::RESULT_PASS.icon();
+}
+
 QVariant TestResultItem::data(int column, int role) const
 {
     switch (role) {
@@ -96,6 +94,8 @@ QVariant TestResultItem::data(int column, int role) const
         const ResultType result = m_testResult->result();
         if (result == ResultType::MessageLocation && parent())
             return parent()->data(column, role);
+        if (result == ResultType::TestStart)
+            return testSummaryIcon(m_summaryResult);
         return testResultIcon(result);
     }
     case Qt::DisplayRole:
@@ -112,26 +112,47 @@ void TestResultItem::updateDescription(const QString &description)
     m_testResult->setDescription(description);
 }
 
-void TestResultItem::updateResult(bool &changed, ResultType addedChildType)
+static bool isSignificant(ResultType type)
+{
+    switch (type) {
+    case ResultType::Skip:
+    case ResultType::Benchmark:
+    case ResultType::MessageInfo:
+    case ResultType::MessageInternal:
+    case ResultType::TestEnd:
+        return false;
+    case ResultType::MessageLocation:
+    case ResultType::MessageCurrentTest:
+    case ResultType::Application:
+    case ResultType::Invalid:
+        QTC_ASSERT_STRING("Got unexpedted type in isSignificant check");
+        return false;
+    default:
+        return true;
+    }
+}
+
+void TestResultItem::updateResult(bool &changed, ResultType addedChildType,
+                                  const Utils::optional<SummaryEvaluation> &summary)
 {
     changed = false;
-    const ResultType old = m_testResult->result();
+    if (m_testResult->result() != ResultType::TestStart)
-    if (old == ResultType::MessageTestCaseFailWarn) // can't become worse
-        return;
-    if (!TestResult::isMessageCaseStart(old))
         return;
 
-    ResultType newResult = old;
+    if (!isSignificant(addedChildType) || (addedChildType == ResultType::TestStart && !summary))
+        return;
+
+    if (m_summaryResult.has_value() && m_summaryResult->failed && m_summaryResult->warnings)
+        return; // can't become worse
+
+    SummaryEvaluation newResult = m_summaryResult.value_or(SummaryEvaluation());
     switch (addedChildType) {
     case ResultType::Fail:
     case ResultType::MessageFatal:
     case ResultType::UnexpectedPass:
-    case ResultType::MessageTestCaseFail:
+        if (newResult.failed)
-        newResult = (old == ResultType::MessageTestCaseSuccessWarn) ? ResultType::MessageTestCaseFailWarn
+            return;
-                                                                    : ResultType::MessageTestCaseFail;
+        newResult.failed = true;
-        break;
-    case ResultType::MessageTestCaseFailWarn:
-        newResult = ResultType::MessageTestCaseFailWarn;
         break;
     case ResultType::ExpectedFail:
     case ResultType::MessageWarn:
@@ -141,20 +162,23 @@ void TestResultItem::updateResult(bool &changed, ResultType addedChildType)
     case ResultType::BlacklistedPass:
     case ResultType::BlacklistedXFail:
     case ResultType::BlacklistedXPass:
-    case ResultType::MessageTestCaseSuccessWarn:
+        if (newResult.warnings)
-        newResult = (old == ResultType::MessageTestCaseFail) ? ResultType::MessageTestCaseFailWarn
+            return;
-                                                             : ResultType::MessageTestCaseSuccessWarn;
+        newResult.warnings = true;
         break;
-    case ResultType::Pass:
+    case ResultType::TestStart:
-    case ResultType::MessageTestCaseSuccess:
+        if (summary) {
-        newResult = (old == ResultType::TestStart) ? ResultType::MessageTestCaseSuccess : old;
+            newResult.failed |= summary->failed;
+            newResult.warnings |= summary->warnings;
+        }
         break;
     default:
         break;
     }
-    changed = old != newResult;
+    changed = !m_summaryResult.has_value() || m_summaryResult.value() != newResult;
+
     if (changed)
-        m_testResult->setResult(newResult);
+        m_summaryResult.emplace(newResult);
 }
 
 TestResultItem *TestResultItem::intermediateFor(const TestResultItem *item) const
@@ -182,6 +206,15 @@ TestResultItem *TestResultItem::createAndAddIntermediateFor(const TestResultItem
     return intermediate;
 }
 
+QString TestResultItem::resultString() const
+{
+    if (testResult()->result() != ResultType::TestStart)
+        return TestResult::resultToString(testResult()->result());
+    if (!m_summaryResult)
+        return QString();
+    return m_summaryResult->failed ? QString("FAIL") : QString("PASS");
+}
+
 /********************************* TestResultModel *****************************************/
 
 TestResultModel::TestResultModel(QObject *parent)
@@ -197,7 +230,7 @@ void TestResultModel::updateParent(const TestResultItem *item)
     if (parentItem == rootItem()) // do not update invisible root item
         return;
     bool changed = false;
-    parentItem->updateResult(changed, item->testResult()->result());
+    parentItem->updateResult(changed, item->testResult()->result(), item->summaryResult());
     if (!changed)
         return;
     emit dataChanged(parentItem->index(), parentItem->index());
@@ -388,10 +421,7 @@ void TestResultFilterModel::enableAllResultTypes()
               << ResultType::MessageFatal << ResultType::Invalid << ResultType::BlacklistedPass
               << ResultType::BlacklistedFail << ResultType::BlacklistedXFail << ResultType::BlacklistedXPass
               << ResultType::Benchmark
-              << ResultType::MessageCurrentTest << ResultType::TestStart
+              << ResultType::MessageCurrentTest << ResultType::TestStart << ResultType::TestEnd
-              << ResultType::MessageTestCaseSuccess << ResultType::MessageTestCaseSuccessWarn
-              << ResultType::MessageTestCaseFail << ResultType::MessageTestCaseFailWarn
-              << ResultType::TestEnd
               << ResultType::MessageInfo << ResultType::MessageSystem << ResultType::Application;
     invalidateFilter();
 }
@@ -433,38 +463,41 @@ const TestResult *TestResultFilterModel::testResult(const QModelIndex &index) co
     return m_sourceModel->testResult(mapToSource(index));
 }
 
+TestResultItem *TestResultFilterModel::itemForIndex(const QModelIndex &index) const
+{
+    return index.isValid() ? m_sourceModel->itemForIndex(mapToSource(index)) : nullptr;
+}
+
 bool TestResultFilterModel::filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const
 {
     QModelIndex index = m_sourceModel->index(sourceRow, 0, sourceParent);
     if (!index.isValid())
         return false;
     ResultType resultType = m_sourceModel->testResult(index)->result();
-    switch (resultType) {
+    if (resultType == ResultType::TestStart) {
-    case ResultType::MessageTestCaseSuccess:
+        TestResultItem *item = m_sourceModel->itemForIndex(index);
-        return m_enabled.contains(ResultType::Pass);
+        QTC_ASSERT(item, return false);
-    case ResultType::MessageTestCaseFail:
+        if (!item->summaryResult())
-    case ResultType::MessageTestCaseSuccessWarn:
+            return true;
-    case ResultType::MessageTestCaseFailWarn:
         return acceptTestCaseResult(index);
-    default:
-        return m_enabled.contains(resultType);
     }
+    return m_enabled.contains(resultType);
 }
 
 bool TestResultFilterModel::acceptTestCaseResult(const QModelIndex &srcIndex) const
 {
     for (int row = 0, count = m_sourceModel->rowCount(srcIndex); row < count; ++row) {
         const QModelIndex &child = m_sourceModel->index(row, 0, srcIndex);
-        ResultType type = m_sourceModel->testResult(child)->result();
+        TestResultItem *item = m_sourceModel->itemForIndex(child);
-        if (type == ResultType::MessageTestCaseSuccess)
+        ResultType type = item->testResult()->result();
-            type = ResultType::Pass;
+
-        if (type == ResultType::MessageTestCaseFail || type == ResultType::MessageTestCaseFailWarn
+        if (type == ResultType::TestStart) {
-                || type == ResultType::MessageTestCaseSuccessWarn) {
+            if (!item->summaryResult())
+                return true;
             if (acceptTestCaseResult(child))
                 return true;
-        } else if (m_enabled.contains(type)) {
+        } else if (m_enabled.contains(type))
             return true;
-        }
     }
     return false;
 }

src/plugins/autotest/testresultmodel.h

@@ -32,6 +32,7 @@
 #include <QFont>
 #include <QSet>
 
+#include <utils/optional.h>
 #include <utils/treemodel.h>
 
 namespace Autotest {
@@ -44,13 +45,29 @@ public:
     QVariant data(int column, int role) const override;
     const TestResult *testResult() const { return m_testResult.data(); }
     void updateDescription(const QString &description);
-    void updateResult(bool &changed, ResultType addedChildType);
+
+    struct SummaryEvaluation
+    {
+        bool failed = false;
+        bool warnings = false;
+
+        bool operator==(const SummaryEvaluation &other) const
+        { return failed == other.failed && warnings == other.warnings; }
+        bool operator!=(const SummaryEvaluation &other) const
+        { return !(*this == other); }
+    };
+
+    void updateResult(bool &changed, ResultType addedChildType,
+                      const Utils::optional<SummaryEvaluation> &summary);
 
     TestResultItem *intermediateFor(const TestResultItem *item) const;
     TestResultItem *createAndAddIntermediateFor(const TestResultItem *child);
+    QString resultString() const;
+    Utils::optional<SummaryEvaluation> summaryResult() const { return m_summaryResult; }
 
 private:
     TestResultPtr m_testResult;
+    Utils::optional<SummaryEvaluation> m_summaryResult;
 };
 
 class TestResultModel : public Utils::TreeModel<TestResultItem>
@@ -96,6 +113,7 @@ public:
     void clearTestResults();
     bool hasResults();
     const TestResult *testResult(const QModelIndex &index) const;
+    TestResultItem *itemForIndex(const QModelIndex &index) const;
 
 protected:
     bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;

src/plugins/autotest/testresultspane.cpp

@@ -647,7 +647,8 @@ QString TestResultsPane::getWholeOutput(const QModelIndex &parent)
         QModelIndex current = m_model->index(row, 0, parent);
         const TestResult *result = m_model->testResult(current);
         QTC_ASSERT(result, continue);
-        output.append(TestResult::resultToString(result->result())).append('\t');
+        if (auto item = m_model->itemForIndex(current))
+            output.append(item->resultString()).append('\t');
         output.append(result->outputString(true)).append('\n');
         output.append(getWholeOutput(current));
     }