Make counts of various types of test result add up correctly

Added tests for repeated skips and failures (from within void lambdas,
to simulate skips and failures from within event handlers). These
exhibit yet more ways to count more than one outcome for a test.  The
new QTest::failOnWarning() can also provoke more than one failure from
a single test, and several existing selftests exhibited various ways
for the Totals line's counts to add up to more than the number of
actual tests run.

Fixed counting so that only the first decisive incident is counted.
Tests can still report later failure or skipping, but only the first
is counted.

Added a currentTestState in qtestlog.cpp, by which it keeps track of
whether the test has resolved to a result, and clearCurrentTestState()
by which other code can reset that at the end of each test. This
brought to light various places where test-end clean-up was not being
handled - due to failure or skipping in a *_data() method or init, or
a skip in cleanup.

Fixes: QTBUG-95661
Change-Id: I5d24a37a53d3db225fa602649d8aad8f5ed6c1ad
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

4 files changed, 95 insertions, 27 deletions

diff --git a/src/testlib/qtestcase.cpp b/src/testlib/qtestcase.cpp
index 9a93c41b5c..673605307f 100644
--- a/src/testlib/qtestcase.cpp
+++ b/src/testlib/qtestcase.cpp
@@ -946,33 +946,38 @@ void TestMethods::invokeTestOnData(int index) const
         do {
             if (m_initMethod.isValid())
                 m_initMethod.invoke(QTest::currentTestObject, Qt::DirectConnection);
-            if (QTestResult::skipCurrentTest() || QTestResult::currentTestFailed())
-                break;
 
-            QBenchmarkTestMethodData::current->result = QBenchmarkResult();
-            QBenchmarkTestMethodData::current->resultAccepted = false;
+            const bool initQuit =
+                QTestResult::skipCurrentTest() || QTestResult::currentTestFailed();
+            if (!initQuit) {
+                QBenchmarkTestMethodData::current->result = QBenchmarkResult();
+                QBenchmarkTestMethodData::current->resultAccepted = false;
 
-            QBenchmarkGlobalData::current->context.tag =
-                QLatin1String(
-                    QTestResult::currentDataTag()
-                    ? QTestResult::currentDataTag() : "");
+                QBenchmarkGlobalData::current->context.tag = QLatin1String(
+                    QTestResult::currentDataTag() ? QTestResult::currentDataTag() : "");
 
-            invokeOk = m_methods[index].invoke(QTest::currentTestObject, Qt::DirectConnection);
-            if (!invokeOk)
-                QTestResult::addFailure("Unable to execute slot", __FILE__, __LINE__);
+                invokeOk = m_methods[index].invoke(QTest::currentTestObject, Qt::DirectConnection);
+                if (!invokeOk)
+                    QTestResult::addFailure("Unable to execute slot", __FILE__, __LINE__);
 
-            isBenchmark = QBenchmarkTestMethodData::current->isBenchmark();
+                isBenchmark = QBenchmarkTestMethodData::current->isBenchmark();
+            } else {
+                invokeOk = false;
+            }
 
             QTestResult::finishedCurrentTestData();
 
-            if (m_cleanupMethod.isValid())
-                m_cleanupMethod.invoke(QTest::currentTestObject, Qt::DirectConnection);
+            if (!initQuit) {
+                if (m_cleanupMethod.isValid())
+                    m_cleanupMethod.invoke(QTest::currentTestObject, Qt::DirectConnection);
 
-            // Process any deleteLater(), like event-loop based apps would do. Fixes memleak reports.
-            if (QCoreApplication::instance())
-                QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
-
-            // If the test isn't a benchmark, finalize the result after cleanup() has finished.
+                // Process any deleteLater(), used by event-loop-based apps.
+                // Fixes memleak reports.
+                if (QCoreApplication::instance())
+                    QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
+            }
+            // If the test isn't a benchmark, finalize the result after
+            // cleanup() has finished (or init has lead us to skip the test).
             if (!isBenchmark)
                 QTestResult::finishedCurrentTestDataCleanup();
 
@@ -1522,7 +1527,7 @@ void TestMethods::invokeTests(QObject *testObject) const
 
     QSignalDumper::startDump();
 
-    if (!QTestResult::skipCurrentTest() && !QTest::currentTestFailed()) {
+    if (!QTestResult::skipCurrentTest() && !QTestResult::currentTestFailed()) {
         if (m_initTestCaseMethod.isValid())
             m_initTestCaseMethod.invoke(testObject, Qt::DirectConnection);
 
@@ -1544,12 +1549,15 @@ void TestMethods::invokeTests(QObject *testObject) const
             }
         }
 
+        const bool wasSkipped = QTestResult::skipCurrentTest();
         QTestResult::setSkipCurrentTest(false);
         QTestResult::setBlacklistCurrentTest(false);
         QTestResult::setCurrentTestFunction("cleanupTestCase");
         if (m_cleanupTestCaseMethod.isValid())
             m_cleanupTestCaseMethod.invoke(testObject, Qt::DirectConnection);
         QTestResult::finishedCurrentTestData();
+        // Restore skip state as it affects decision on whether we passed:
+        QTestResult::setSkipCurrentTest(wasSkipped || QTestResult::skipCurrentTest());
         QTestResult::finishedCurrentTestDataCleanup();
     }
     QTestResult::finishedCurrentTestFunction();
diff --git a/src/testlib/qtestlog.cpp b/src/testlib/qtestlog.cpp
index c90850e786..4f0e3a43bc 100644
--- a/src/testlib/qtestlog.cpp
+++ b/src/testlib/qtestlog.cpp
@@ -111,6 +111,7 @@ namespace QTest {
     int passes = 0;
     int skips = 0;
     int blacklists = 0;
+    enum { Unresolved, Passed, Skipped, Suppressed, Failed } currentTestState;
 
     struct IgnoreResultList
     {
@@ -345,19 +346,27 @@ void QTestLog::clearIgnoreMessages()
     QTest::IgnoreResultList::clearList(QTest::ignoreResultList);
 }
 
+
 void QTestLog::clearFailOnWarnings()
 {
     QTest::failOnWarningList.clear();
 }
 
+void QTestLog::clearCurrentTestState()
+{
+    QTest::currentTestState = QTest::Unresolved;
+}
+
 void QTestLog::addPass(const char *msg)
 {
     if (printAvailableTags)
         return;
 
     QTEST_ASSERT(msg);
+    Q_ASSERT(QTest::currentTestState == QTest::Unresolved);
 
     ++QTest::passes;
+    QTest::currentTestState = QTest::Passed;
 
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::Pass, msg);
@@ -367,8 +376,18 @@ void QTestLog::addFail(const char *msg, const char *file, int line)
 {
     QTEST_ASSERT(msg);
 
-    ++QTest::fails;
+    if (QTest::currentTestState == QTest::Unresolved) {
+        ++QTest::fails;
+    } else {
+        // After an XPASS/Continue, or fail or skip in a function the test
+        // calls, we can subsequently fail.
+        Q_ASSERT(QTest::currentTestState == QTest::Failed
+                 || QTest::currentTestState == QTest::Skipped);
+    }
+    // It is up to particular loggers to decide whether to report such
+    // subsequent failures; they may carry useful information.
 
+    QTest::currentTestState = QTest::Failed;
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::Fail, msg, file, line);
 }
@@ -387,8 +406,16 @@ void QTestLog::addXPass(const char *msg, const char *file, int line)
 {
     QTEST_ASSERT(msg);
 
-    ++QTest::fails;
+    if (QTest::currentTestState == QTest::Unresolved) {
+        ++QTest::fails;
+    } else {
+        // After an XPASS/Continue, we can subsequently XPASS again.
+        // Likewise after a fail or skip in a function called by the test.
+        Q_ASSERT(QTest::currentTestState == QTest::Failed
+                 || QTest::currentTestState == QTest::Skipped);
+    }
 
+    QTest::currentTestState = QTest::Failed;
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::XPass, msg, file, line);
 }
@@ -396,8 +423,10 @@ void QTestLog::addXPass(const char *msg, const char *file, int line)
 void QTestLog::addBPass(const char *msg)
 {
     QTEST_ASSERT(msg);
+    Q_ASSERT(QTest::currentTestState == QTest::Unresolved);
 
-    ++QTest::blacklists;
+    ++QTest::blacklists; // Not passes ?
+    QTest::currentTestState = QTest::Suppressed;
 
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::BlacklistedPass, msg);
@@ -407,8 +436,16 @@ void QTestLog::addBFail(const char *msg, const char *file, int line)
 {
     QTEST_ASSERT(msg);
 
-    ++QTest::blacklists;
+    if (QTest::currentTestState == QTest::Unresolved) {
+        ++QTest::blacklists;
+    } else {
+        // After a BXPASS/Continue, we can subsequently fail.
+        // Likewise after a fail or skip in a function called by a test.
+        Q_ASSERT(QTest::currentTestState == QTest::Suppressed
+                 || QTest::currentTestState == QTest::Skipped);
+    }
 
+    QTest::currentTestState = QTest::Suppressed;
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::BlacklistedFail, msg, file, line);
 }
@@ -417,8 +454,16 @@ void QTestLog::addBXPass(const char *msg, const char *file, int line)
 {
     QTEST_ASSERT(msg);
 
-    ++QTest::blacklists;
+    if (QTest::currentTestState == QTest::Unresolved) {
+        ++QTest::blacklists;
+    } else {
+        // After a BXPASS/Continue, we may BXPASS again.
+        // Likewise after a fail or skip in a function called by a test.
+        Q_ASSERT(QTest::currentTestState == QTest::Suppressed
+                 || QTest::currentTestState == QTest::Skipped);
+    }
 
+    QTest::currentTestState = QTest::Suppressed;
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::BlacklistedXPass, msg, file, line);
 }
@@ -437,7 +482,18 @@ void QTestLog::addSkip(const char *msg, const char *file, int line)
 {
     QTEST_ASSERT(msg);
 
-    ++QTest::skips;
+    if (QTest::currentTestState == QTest::Unresolved) {
+        ++QTest::skips;
+        QTest::currentTestState = QTest::Skipped;
+    } else {
+        // After an B?XPASS/Continue, we might subsequently skip.
+        // Likewise after a skip in a function called by a test.
+        Q_ASSERT(QTest::currentTestState == QTest::Suppressed
+                 || QTest::currentTestState == QTest::Failed
+                 || QTest::currentTestState == QTest::Skipped);
+    }
+    // It is up to particular loggers to decide whether to report such
+    // subsequent skips; they may carry useful information.
 
     FOREACH_TEST_LOGGER
         logger->addIncident(QAbstractTestLogger::Skip, msg, file, line);
diff --git a/src/testlib/qtestlog_p.h b/src/testlib/qtestlog_p.h
index cc24360bed..36e38a33cb 100644
--- a/src/testlib/qtestlog_p.h
+++ b/src/testlib/qtestlog_p.h
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2021 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtTest module of the Qt Toolkit.
@@ -113,6 +113,7 @@ public:
     static void printUnhandledIgnoreMessages();
     static void clearIgnoreMessages();
     static void clearFailOnWarnings();
+    static void clearCurrentTestState();
 
     static void warn(const char *msg, const char *file, int line);
     static void info(const char *msg, const char *file, int line);
diff --git a/src/testlib/qtestresult.cpp b/src/testlib/qtestresult.cpp
index ae40164392..a3a0bd08b3 100644
--- a/src/testlib/qtestresult.cpp
+++ b/src/testlib/qtestresult.cpp
@@ -219,6 +219,7 @@ void QTestResult::finishedCurrentTestDataCleanup()
             QTestLog::addPass("");
     }
 
+    QTestLog::clearCurrentTestState();
     QTest::resetFailed();
 }
 
@@ -232,6 +233,7 @@ void QTestResult::finishedCurrentTestDataCleanup()
 */
 void QTestResult::finishedCurrentTestFunction()
 {
+    QTestLog::clearCurrentTestState(); // Needed if _data() skipped.
     QTestLog::leaveTestFunction();
 
     QTest::currentTestFunc = nullptr;
@@ -296,6 +298,7 @@ static bool checkStatement(bool statement, const char *msg, const char *file, in
                 QTestLog::addXPass(msg, file, line);
 
             QTest::setFailed(true);
+            // Should B?XPass always (a) continue or (b) abort, regardless of mode ?
             bool doContinue = (QTest::expectFailMode == QTest::Continue);
             clearExpectFail();
             return doContinue;