TestResult: Devirtualize the class - part 1 of 5

The goal is to make the TestResult a value type without a need
to create these objects on heap.

Introduce a ResultHooks class that will replace all 5 virtual
methods.

Step 1 - implement outputStringHook.

Reorder constructor arguments of TestResult's subclasses so that
the first 2 args are always: id and name.

Change-Id: Iae93e5a348328414f057d1471afad8020b3c1171
Reviewed-by: Christian Stenger <christian.stenger@qt.io>

src/plugins/autotest/boost/boosttestoutputreader.cpp

@@ -74,9 +74,8 @@ static QString caseFromContent(const QString &content)
 void BoostTestOutputReader::sendCompleteInformation()
 {
     QTC_ASSERT(m_result != ResultType::Invalid, return);
-    BoostTestResult *result = new BoostTestResult(id(), m_projectFile, m_currentModule);
+    BoostTestResult *result = new BoostTestResult(id(), m_currentModule, m_projectFile,
-    result->setTestSuite(m_currentSuite);
+                                                  m_currentTest, m_currentSuite);
-    result->setTestCase(m_currentTest);
     if (m_lineNumber) {
         result->setLine(m_lineNumber);
         result->setFileName(m_fileName);
@@ -213,7 +212,7 @@ void BoostTestOutputReader::processOutputLine(const QByteArray &outputLine)
     if (match.hasMatch()) {
         if (m_result != ResultType::Invalid)
             sendCompleteInformation();
-        BoostTestResult *result = new BoostTestResult(id(), m_projectFile, m_currentModule);
+        BoostTestResult *result = new BoostTestResult(id(), m_currentModule, m_projectFile);
         result->setDescription(match.captured(0));
         result->setResult(ResultType::MessageInfo);
         reportResult(TestResultPtr(result));
@@ -235,14 +234,14 @@ void BoostTestOutputReader::processOutputLine(const QByteArray &outputLine)
             sendCompleteInformation();
         if (match.captured(1).startsWith("Entering")) {
             m_currentModule = match.captured(2);
-            BoostTestResult *result = new BoostTestResult(id(), m_projectFile, m_currentModule);
+            BoostTestResult *result = new BoostTestResult(id(), m_currentModule, m_projectFile);
             result->setDescription(Tr::tr("Executing test module %1").arg(m_currentModule));
             result->setResult(ResultType::TestStart);
             reportResult(TestResultPtr(result));
             m_description.clear();
         } else {
             QTC_CHECK(m_currentModule == match.captured(3));
-            BoostTestResult *result = new BoostTestResult(id(), m_projectFile, m_currentModule);
+            BoostTestResult *result = new BoostTestResult(id(), m_currentModule, m_projectFile);
             result->setDescription(Tr::tr("Test module execution took %1").arg(match.captured(4)));
             result->setResult(ResultType::TestEnd);
             reportResult(TestResultPtr(result));
@@ -326,7 +325,7 @@ void BoostTestOutputReader::processOutputLine(const QByteArray &outputLine)
     if (match.hasMatch()) {
         if (m_result != ResultType::Invalid)
             sendCompleteInformation();
-        BoostTestResult *result = new BoostTestResult(id(), m_projectFile, QString());
+        BoostTestResult *result = new BoostTestResult(id(), {}, m_projectFile);
         int failed = match.captured(1).toInt();
         int fatals = m_summary.value(ResultType::MessageFatal);
         QString txt = Tr::tr("%1 failures detected in %2.").arg(failed).arg(match.captured(3));
@@ -347,7 +346,7 @@ void BoostTestOutputReader::processOutputLine(const QByteArray &outputLine)
     if (line == noErrors) {
         if (m_result != ResultType::Invalid)
             sendCompleteInformation();
-        BoostTestResult *result = new BoostTestResult(id(), m_projectFile, QString());
+        BoostTestResult *result = new BoostTestResult(id(), {}, m_projectFile);
         QString txt = Tr::tr("No errors detected.");
         if (m_testCaseCount != -1)
             txt.append(' ').append(Tr::tr("%1 tests passed.").arg(m_testCaseCount));
@@ -375,11 +374,8 @@ void BoostTestOutputReader::processStdError(const QByteArray &outputLine)
 
 TestResultPtr BoostTestOutputReader::createDefaultResult() const
 {
-    BoostTestResult *result = new BoostTestResult(id(), m_projectFile, m_currentModule);
+    return TestResultPtr(new BoostTestResult(id(), m_currentModule, m_projectFile,
-    result->setTestSuite(m_currentSuite);
+                                             m_currentTest, m_currentSuite));
-    result->setTestCase(m_currentTest);
-
-    return TestResultPtr(result);
 }
 
 void BoostTestOutputReader::onDone() {
@@ -420,8 +416,8 @@ void BoostTestOutputReader::onDone() {
 
 void BoostTestOutputReader::reportNoOutputFinish(const QString &description, ResultType type)
 {
-    BoostTestResult *result = new BoostTestResult(id(), m_projectFile, m_currentModule);
+    BoostTestResult *result = new BoostTestResult(id(), m_currentModule, m_projectFile,
-    result->setTestCase(Tr::tr("Running tests without output."));
+                                                  Tr::tr("Running tests without output."));
     result->setDescription(description);
     result->setResult(type);
     reportResult(TestResultPtr(result));

src/plugins/autotest/boost/boosttestoutputreader.h

@@ -8,7 +8,6 @@
 namespace Autotest {
 namespace Internal {
 
-class BoostTestResult;
 enum class LogLevel;
 enum class ReportLevel;
 

src/plugins/autotest/boost/boosttestresult.cpp

@@ -14,19 +14,15 @@
 namespace Autotest {
 namespace Internal {
 
-BoostTestResult::BoostTestResult(const QString &id, const Utils::FilePath &projectFile, const QString &name)
+static ResultHooks::OutputStringHook outputStringHook(const QString &testCaseName)
-    : TestResult(id, name), m_projectFile(projectFile)
 {
-}
+    return [testCaseName](const TestResult &result, bool selected) {
-
+        const QString &desc = result.description();
-const QString BoostTestResult::outputString(bool selected) const
-{
-    const QString &desc = description();
         QString output;
-    switch (result()) {
+        switch (result.result()) {
         case ResultType::Pass:
         case ResultType::Fail:
-        output = m_testCase;
+            output = testCaseName;
             if (selected && !desc.isEmpty())
                 output.append('\n').append(desc);
             break;
@@ -36,6 +32,17 @@ const QString BoostTestResult::outputString(bool selected) const
                 output = output.split('\n').first();
         }
         return output;
+    };
+}
+
+BoostTestResult::BoostTestResult(const QString &id, const QString &name,
+                                 const Utils::FilePath &projectFile,
+                                 const QString &testCaseName, const QString &testSuiteName)
+    : TestResult(id, name, {outputStringHook(testCaseName)})
+    , m_projectFile(projectFile)
+    , m_testCaseName(testCaseName)
+    , m_testSuiteName(testSuiteName)
+{
 }
 
 bool BoostTestResult::isDirectParentOf(const TestResult *other, bool *needsIntermediate) const
@@ -46,24 +53,24 @@ bool BoostTestResult::isDirectParentOf(const TestResult *other, bool *needsInter
     if (result() != ResultType::TestStart)
         return false;
 
-    bool weAreModule = (m_testCase.isEmpty() && m_testSuite.isEmpty());
+    bool weAreModule = (m_testCaseName.isEmpty() && m_testSuiteName.isEmpty());
-    bool weAreSuite = (m_testCase.isEmpty() && !m_testSuite.isEmpty());
+    bool weAreSuite = (m_testCaseName.isEmpty() && !m_testSuiteName.isEmpty());
-    bool weAreCase = (!m_testCase.isEmpty());
+    bool weAreCase = (!m_testCaseName.isEmpty());
 
     const BoostTestResult *boostOther = static_cast<const BoostTestResult *>(other);
-    bool otherIsSuite = boostOther->m_testCase.isEmpty() && !boostOther->m_testSuite.isEmpty();
+    bool otherIsSuite = boostOther->m_testCaseName.isEmpty() && !boostOther->m_testSuiteName.isEmpty();
-    bool otherIsCase = !boostOther->m_testCase.isEmpty();
+    bool otherIsCase = !boostOther->m_testCaseName.isEmpty();
 
     if (otherIsSuite)
-        return weAreSuite ? boostOther->m_testSuite.startsWith(m_testSuite + '/') : weAreModule;
+        return weAreSuite ? boostOther->m_testSuiteName.startsWith(m_testSuiteName + '/') : weAreModule;
 
     if (otherIsCase) {
         if (weAreCase)
-            return boostOther->m_testCase == m_testCase && boostOther->m_testSuite == m_testSuite;
+            return boostOther->m_testCaseName == m_testCaseName && boostOther->m_testSuiteName == m_testSuiteName;
         if (weAreSuite)
-            return boostOther->m_testSuite == m_testSuite;
+            return boostOther->m_testSuiteName == m_testSuiteName;
         if (weAreModule)
-            return boostOther->m_testSuite.isEmpty();
+            return boostOther->m_testSuiteName.isEmpty();
     }
     return false;
 }
@@ -88,16 +95,16 @@ bool BoostTestResult::matches(const BoostTestTreeItem *item) const
     // might end up here with a differing set of tests, but it's the best we can do
     if (!item)
         return false;
-    if (m_testCase.isEmpty()) // a top level module node
+    if (m_testCaseName.isEmpty()) // a top level module node
         return item->proFile() == m_projectFile;
     if (item->proFile() != m_projectFile)
         return false;
     if (!fileName().isEmpty() && fileName() != item->filePath())
         return false;
 
-    QString fullName = "::" + m_testCase;
+    QString fullName = "::" + m_testCaseName;
-    fullName.prepend(m_testSuite.isEmpty() ? QString(BoostTest::Constants::BOOST_MASTER_SUITE)
+    fullName.prepend(m_testSuiteName.isEmpty() ? QString(BoostTest::Constants::BOOST_MASTER_SUITE)
-                                           : m_testSuite);
+                                           : m_testSuiteName);
 
     BoostTestTreeItem::TestStates states = item->state();
     if (states & BoostTestTreeItem::Templated) {

src/plugins/autotest/boost/boosttestresult.h

@@ -13,19 +13,17 @@ class BoostTestTreeItem;
 class BoostTestResult : public TestResult
 {
 public:
-    BoostTestResult(const QString &id, const Utils::FilePath &projectFile, const QString &name);
+    BoostTestResult(const QString &id, const QString &name, const Utils::FilePath &projectFile,
-    const QString outputString(bool selected) const override;
+                    const QString &testCaseName = {}, const QString &testSuiteName = {});
 
     bool isDirectParentOf(const TestResult *other, bool *needsIntermediate) const override;
     const ITestTreeItem * findTestTreeItem() const override;
-    void setTestSuite(const QString &testSuite) { m_testSuite = testSuite; }
-    void setTestCase(const QString &testCase) { m_testCase = testCase; }
 private:
     bool matches(const BoostTestTreeItem *item) const;
 
     Utils::FilePath m_projectFile;
-    QString m_testSuite;
+    QString m_testCaseName;
-    QString m_testCase;
+    QString m_testSuiteName;
 };
 
 } // namespace Internal

src/plugins/autotest/gtest/gtestoutputreader.cpp

@@ -70,7 +70,7 @@ void GTestOutputReader::processOutputLine(const QByteArray &outputLine)
             m_description = line;
             if (m_iteration > 1)
                 m_description.append(' ' + Tr::tr("(iteration %1)").arg(m_iteration));
-            TestResultPtr testResult = TestResultPtr(new GTestResult(id(), m_projectFile, QString()));
+            TestResultPtr testResult = TestResultPtr(new GTestResult(id(), {}, m_projectFile));
             testResult->setResult(ResultType::MessageInternal);
             testResult->setDescription(m_description);
             reportResult(testResult);
@@ -103,8 +103,7 @@ void GTestOutputReader::processOutputLine(const QByteArray &outputLine)
     } else if (ExactMatch match = newTestSetStarts.match(line)) {
         m_testSetStarted = true;
         setCurrentTestCase(match.captured(1));
-        TestResultPtr testResult = TestResultPtr(new GTestResult(QString(), m_projectFile,
+        TestResultPtr testResult = TestResultPtr(new GTestResult({}, {}, m_projectFile));
-                                                                 QString()));
         testResult->setResult(ResultType::MessageCurrentTest);
         testResult->setDescription(Tr::tr("Entering test case %1").arg(m_currentTestCase));
         reportResult(testResult);
@@ -178,9 +177,8 @@ void GTestOutputReader::processStdError(const QByteArray &outputLine)
 
 TestResultPtr GTestOutputReader::createDefaultResult() const
 {
-    GTestResult *result = new GTestResult(id(), m_projectFile, m_currentTestSuite);
+    GTestResult *result = new GTestResult(id(), m_currentTestSuite, m_projectFile,
-    result->setTestCaseName(m_currentTestCase);
+                                          m_currentTestCase, m_iteration);
-    result->setIteration(m_iteration);
 
     const ITestTreeItem *testItem = result->findTestTreeItem();
 

src/plugins/autotest/gtest/gtestresult.cpp

@@ -14,20 +14,15 @@
 namespace Autotest {
 namespace Internal {
 
-GTestResult::GTestResult(const QString &id, const Utils::FilePath &projectFile,
+static ResultHooks::OutputStringHook outputStringHook(const QString &testCaseName)
-                         const QString &name)
-    : TestResult(id, name), m_projectFile(projectFile)
 {
-}
+    return [testCaseName](const TestResult &result, bool selected) {
-
+        const QString &desc = result.description();
-const QString GTestResult::outputString(bool selected) const
-{
-    const QString &desc = description();
         QString output;
-    switch (result()) {
+        switch (result.result()) {
         case ResultType::Pass:
         case ResultType::Fail:
-        output = m_testCaseName;
+            output = testCaseName;
             if (selected && !desc.isEmpty())
                 output.append('\n').append(desc);
             break;
@@ -37,6 +32,16 @@ const QString GTestResult::outputString(bool selected) const
                 output = output.split('\n').first();
         }
         return output;
+    };
+}
+
+GTestResult::GTestResult(const QString &id, const QString &name, const Utils::FilePath &projectFile,
+                         const QString &testCaseName, int iteration)
+    : TestResult(id, name, {outputStringHook(testCaseName)})
+    , m_projectFile(projectFile)
+    , m_testCaseName(testCaseName)
+    , m_iteration(iteration)
+{
 }
 
 bool GTestResult::isDirectParentOf(const TestResult *other, bool *needsIntermediate) const

src/plugins/autotest/gtest/gtestresult.h

@@ -14,11 +14,9 @@ namespace Internal {
 class GTestResult : public TestResult
 {
 public:
-    GTestResult(const QString &id, const Utils::FilePath &projectFile, const QString &name);
+    GTestResult(const QString &id, const QString &name, const Utils::FilePath &projectFile,
-    const QString outputString(bool selected) const override;
+                const QString &testCaseName = {}, int iteration = 1);
 
-    void setTestCaseName(const QString &testSetName) { m_testCaseName = testSetName; }
-    void setIteration(int iteration) { m_iteration = iteration; }
     bool isDirectParentOf(const TestResult *other, bool *needsIntermediate) const override;
     virtual const ITestTreeItem *findTestTreeItem() const override;
 
@@ -30,8 +28,8 @@ private:
     bool matchesTestCase(const TestTreeItem *treeItem) const;
     bool matchesTestSuite(const TestTreeItem *treeItem) const;
 
-    QString m_testCaseName;
     Utils::FilePath m_projectFile;
+    QString m_testCaseName;
     int m_iteration = 1;
 };
 

src/plugins/autotest/qtest/qttestoutputreader.cpp

@@ -132,9 +132,8 @@ void QtTestOutputReader::processOutputLine(const QByteArray &outputLine)
 
 TestResultPtr QtTestOutputReader::createDefaultResult() const
 {
-    QtTestResult *result = new QtTestResult(id(), m_projectFile, m_testType, m_className);
+    QtTestResult *result = new QtTestResult(id(), m_className, m_projectFile, m_testType,
-    result->setFunctionName(m_testCase);
+                                            m_testCase, m_dataTag);
-    result->setDataTag(m_dataTag);
     return TestResultPtr(result);
 }
 
@@ -475,7 +474,7 @@ void QtTestOutputReader::sendCompleteInformation()
 
 void QtTestOutputReader::sendMessageCurrentTest()
 {
-    QtTestResult *testResult = new QtTestResult(QString(), m_projectFile, m_testType, QString());
+    QtTestResult *testResult = new QtTestResult({}, {}, m_projectFile, m_testType);
     testResult->setResult(ResultType::MessageCurrentTest);
     testResult->setDescription(Tr::tr("Entering test function %1::%2").arg(m_className, m_testCase));
     reportResult(TestResultPtr(testResult));

src/plugins/autotest/qtest/qttestresult.cpp

@@ -12,18 +12,13 @@
 namespace Autotest {
 namespace Internal {
 
-QtTestResult::QtTestResult(const QString &id, const Utils::FilePath &projectFile, TestType type,
+static ResultHooks::OutputStringHook outputStringHook(const QString &function, const QString &dataTag)
-                           const QString &className)
-    : TestResult(id, className), m_projectFile(projectFile), m_type(type)
 {
-}
+    return [function, dataTag](const TestResult &result, bool selected) {
-
+        const QString &desc = result.description();
-const QString QtTestResult::outputString(bool selected) const
+        const QString &className = result.name();
-{
-    const QString &desc = description();
-    const QString &className = name();
         QString output;
-    switch (result()) {
+        switch (result.result()) {
         case ResultType::Pass:
         case ResultType::Fail:
         case ResultType::ExpectedFail:
@@ -31,20 +26,20 @@ const QString QtTestResult::outputString(bool selected) const
         case ResultType::BlacklistedFail:
         case ResultType::BlacklistedPass:
             output = className;
-        if (!m_function.isEmpty())
+            if (!function.isEmpty())
-            output.append("::" + m_function);
+                output.append("::" + function);
-        if (!m_dataTag.isEmpty())
+            if (!dataTag.isEmpty())
-            output.append(QString(" (%1)").arg(m_dataTag));
+                output.append(QString(" (%1)").arg(dataTag));
             if (selected && !desc.isEmpty()) {
                 output.append('\n').append(desc);
             }
             break;
         case ResultType::Benchmark:
             output = className;
-        if (!m_function.isEmpty())
+            if (!function.isEmpty())
-            output.append("::" + m_function);
+                output.append("::" + function);
-        if (!m_dataTag.isEmpty())
+            if (!dataTag.isEmpty())
-            output.append(QString(" (%1)").arg(m_dataTag));
+                output.append(QString(" (%1)").arg(dataTag));
             if (!desc.isEmpty()) {
                 int breakPos = desc.indexOf('(');
                 output.append(": ").append(desc.left(breakPos));
@@ -58,8 +53,18 @@ const QString QtTestResult::outputString(bool selected) const
                 output = output.split('\n').first();
         }
         return output;
+    };
 }
 
+QtTestResult::QtTestResult(const QString &id, const QString &name,
+                           const Utils::FilePath &projectFile, TestType type,
+                           const QString &functionName, const QString &dataTag)
+    : TestResult(id, name, {outputStringHook(functionName, dataTag)})
+    , m_projectFile(projectFile)
+    , m_type(type)
+    , m_function(functionName)
+    , m_dataTag(dataTag) {}
+
 bool QtTestResult::isDirectParentOf(const TestResult *other, bool *needsIntermediate) const
 {
     if (!TestResult::isDirectParentOf(other, needsIntermediate))
@@ -97,8 +102,8 @@ TestResult *QtTestResult::createIntermediateResultFor(const TestResult *other)
 {
     QTC_ASSERT(other, return nullptr);
     const QtTestResult *qtOther = static_cast<const QtTestResult *>(other);
-    QtTestResult *intermediate = new QtTestResult(qtOther->id(), qtOther->m_projectFile,
+    QtTestResult *intermediate = new QtTestResult(qtOther->id(), qtOther->name(), qtOther->m_projectFile,
-                                                  m_type, qtOther->name());
+                                                  m_type);
     intermediate->m_function = qtOther->m_function;
     intermediate->m_dataTag = qtOther->m_dataTag;
     // intermediates will be needed only for data tags

src/plugins/autotest/qtest/qttestresult.h

@@ -15,12 +15,8 @@ namespace Internal {
 class QtTestResult : public TestResult
 {
 public:
-    QtTestResult(const QString &id, const Utils::FilePath &projectFile, TestType type,
+    QtTestResult(const QString &id, const QString &name, const Utils::FilePath &projectFile,
-                 const QString &className);
+                 TestType type, const QString &functionName = {}, const QString &dataTag = {});
-    const QString outputString(bool selected) const override;
-
-    void setFunctionName(const QString &functionName) { m_function = functionName; }
-    void setDataTag(const QString &dataTag) { m_dataTag = dataTag; }
 
     bool isDirectParentOf(const TestResult *other, bool *needsIntermediate) const override;
     bool isIntermediateFor(const TestResult *other) const override;
@@ -35,10 +31,10 @@ private:
     bool matchesTestCase(const TestTreeItem *item) const;
     bool matchesTestFunction(const TestTreeItem *item) const;
 
-    QString m_function;
-    QString m_dataTag;
     Utils::FilePath m_projectFile;
     TestType m_type;
+    QString m_function;
+    QString m_dataTag;
 };
 
 } // namespace Internal

src/plugins/autotest/testresult.cpp

@@ -8,14 +8,18 @@
 
 namespace Autotest {
 
-TestResult::TestResult(const QString &id, const QString &name)
+TestResult::TestResult(const QString &id, const QString &name, const ResultHooks &hooks)
     : m_id(id)
     , m_name(name)
+    , m_hooks(hooks)
 {
 }
 
 const QString TestResult::outputString(bool selected) const
 {
+    if (m_hooks.outputString)
+        return m_hooks.outputString(*this, selected);
+
     if (m_result == ResultType::Application)
         return m_id;
     return selected ? m_description : m_description.split('\n').first();

src/plugins/autotest/testresult.h

@@ -58,14 +58,22 @@ inline auto qHash(const ResultType &result)
     return QT_PREPEND_NAMESPACE(qHash(int(result)));
 }
 
+class TestResult;
+
+struct ResultHooks
+{
+    using OutputStringHook = std::function<QString(const TestResult &, bool)>;
+    OutputStringHook outputString;
+};
+
 class TestResult
 {
 public:
     TestResult() = default;
-    TestResult(const QString &id, const QString &name);
+    TestResult(const QString &id, const QString &name, const ResultHooks &hooks = {});
     virtual ~TestResult() {}
 
-    virtual const QString outputString(bool selected) const;
+    const QString outputString(bool selected) const;
     virtual const ITestTreeItem *findTestTreeItem() const;
 
     QString id() const { return m_id; }
@@ -95,6 +103,7 @@ private:
     QString m_description;
     Utils::FilePath m_file;
     int m_line = 0;
+    ResultHooks m_hooks;
 };
 
 using TestResultPtr = QSharedPointer<TestResult>;