From 0576e419e380ce58ffe96ff37e527d3eccbcc01f Mon Sep 17 00:00:00 2001 From: Christian Stenger Date: Mon, 15 Apr 2019 16:08:10 +0200 Subject: [PATCH] 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 --- src/plugins/autotest/qtest/qttestresult.cpp | 2 +- src/plugins/autotest/testresult.cpp | 11 -- src/plugins/autotest/testresult.h | 7 +- src/plugins/autotest/testresultdelegate.cpp | 6 +- src/plugins/autotest/testresultmodel.cpp | 137 ++++++++++++-------- src/plugins/autotest/testresultmodel.h | 20 ++- src/plugins/autotest/testresultspane.cpp | 3 +- 7 files changed, 112 insertions(+), 74 deletions(-) diff --git a/src/plugins/autotest/qtest/qttestresult.cpp b/src/plugins/autotest/qtest/qttestresult.cpp index e59dccdb92c..6644f61a6cb 100644 --- a/src/plugins/autotest/qtest/qttestresult.cpp +++ b/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(other); - if (TestResult::isMessageCaseStart(result())) { + if (result() == ResultType::TestStart) { if (qtOther->isDataTag()) { if (qtOther->m_function == m_function) { if (m_dataTag.isEmpty()) { diff --git a/src/plugins/autotest/testresult.cpp b/src/plugins/autotest/testresult.cpp index 6228c0b4441..e9ff367ef96 100644 --- a/src/plugins/autotest/testresult.cpp +++ b/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); diff --git a/src/plugins/autotest/testresult.h b/src/plugins/autotest/testresult.h index 390b2633199..84fc2c144ae 100644 --- a/src/plugins/autotest/testresult.h +++ b/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; diff --git a/src/plugins/autotest/testresultdelegate.cpp b/src/plugins/autotest/testresultdelegate.cpp index 5d8e1371144..f5486149af6 100644 --- a/src/plugins/autotest/testresultdelegate.cpp +++ b/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())); diff --git a/src/plugins/autotest/testresultmodel.cpp b/src/plugins/autotest/testresultmodel.cpp index 7d803b753a8..f0dd0591532 100644 --- a/src/plugins/autotest/testresultmodel.cpp +++ b/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 &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 &summary) { changed = false; - const ResultType old = m_testResult->result(); - if (old == ResultType::MessageTestCaseFailWarn) // can't become worse - return; - if (!TestResult::isMessageCaseStart(old)) + if (m_testResult->result() != ResultType::TestStart) 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: - newResult = (old == ResultType::MessageTestCaseSuccessWarn) ? ResultType::MessageTestCaseFailWarn - : ResultType::MessageTestCaseFail; - break; - case ResultType::MessageTestCaseFailWarn: - newResult = ResultType::MessageTestCaseFailWarn; + if (newResult.failed) + return; + newResult.failed = true; 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: - newResult = (old == ResultType::MessageTestCaseFail) ? ResultType::MessageTestCaseFailWarn - : ResultType::MessageTestCaseSuccessWarn; + if (newResult.warnings) + return; + newResult.warnings = true; break; - case ResultType::Pass: - case ResultType::MessageTestCaseSuccess: - newResult = (old == ResultType::TestStart) ? ResultType::MessageTestCaseSuccess : old; + case ResultType::TestStart: + if (summary) { + 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::MessageTestCaseSuccess << ResultType::MessageTestCaseSuccessWarn - << ResultType::MessageTestCaseFail << ResultType::MessageTestCaseFailWarn - << ResultType::TestEnd + << ResultType::MessageCurrentTest << ResultType::TestStart << 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) { - case ResultType::MessageTestCaseSuccess: - return m_enabled.contains(ResultType::Pass); - case ResultType::MessageTestCaseFail: - case ResultType::MessageTestCaseSuccessWarn: - case ResultType::MessageTestCaseFailWarn: + if (resultType == ResultType::TestStart) { + TestResultItem *item = m_sourceModel->itemForIndex(index); + QTC_ASSERT(item, return false); + if (!item->summaryResult()) + return true; 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(); - if (type == ResultType::MessageTestCaseSuccess) - type = ResultType::Pass; - if (type == ResultType::MessageTestCaseFail || type == ResultType::MessageTestCaseFailWarn - || type == ResultType::MessageTestCaseSuccessWarn) { + TestResultItem *item = m_sourceModel->itemForIndex(child); + ResultType type = item->testResult()->result(); + + if (type == ResultType::TestStart) { + 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; } diff --git a/src/plugins/autotest/testresultmodel.h b/src/plugins/autotest/testresultmodel.h index f30f6a09509..ea4af1df2c9 100644 --- a/src/plugins/autotest/testresultmodel.h +++ b/src/plugins/autotest/testresultmodel.h @@ -32,6 +32,7 @@ #include #include +#include #include 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 &summary); TestResultItem *intermediateFor(const TestResultItem *item) const; TestResultItem *createAndAddIntermediateFor(const TestResultItem *child); + QString resultString() const; + Utils::optional summaryResult() const { return m_summaryResult; } private: TestResultPtr m_testResult; + Utils::optional m_summaryResult; }; class TestResultModel : public Utils::TreeModel @@ -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; diff --git a/src/plugins/autotest/testresultspane.cpp b/src/plugins/autotest/testresultspane.cpp index 3b4c9e5d21e..22a2ed71630 100644 --- a/src/plugins/autotest/testresultspane.cpp +++ b/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)); }