Make Skip an incident in test logging

Skip ends the test (albeit inconclusively).  Rearrange the enums in
the abstract logger, move code to handle skip between relevant
function and tidy up various things that became simpler as a result.

Also reorder the message enum, and its switches, to separate testlib's
internals from the usual Qt messages, and put each group in ascending
order of severity.

Task-number: QTBUG-96844
Change-Id: I2c7a634b9f849830d64eafa750155e66e244b729
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

9 files changed, 75 insertions, 87 deletions

diff --git a/src/testlib/qabstracttestlogger.cpp b/src/testlib/qabstracttestlogger.cpp
index ce9e0c593a..cb9a049357 100644
--- a/src/testlib/qabstracttestlogger.cpp
+++ b/src/testlib/qabstracttestlogger.cpp
@@ -83,6 +83,7 @@ QT_BEGIN_NAMESPACE
     \value XPass A check which was expected to fail actually passed. This is
            counted as a failure, as it means whatever caused the known failure
            no longer does, so the test needs an update.
+    \value Skip The current test ended prematurely, skipping some checks.
     \value BlacklistedPass As Pass but the test was blacklisted.
     \value BlacklistedXFail As XFail but the test was blacklisted.
     \value BlacklistedFail As Fail but the test was blacklisted.
@@ -122,7 +123,6 @@ QT_BEGIN_NAMESPACE
     \value QCritical A critical error from qCritical().
     \value QFatal A fatal error from qFatal(), or an unrecognised message from
            the Qt logging functions.
-    \value Skip The current test ended prematurely, skipping some checks.
     \value Info Messages QtTest generates as requested by the \c{-v1} or \c{-v2}
            command-line option being specified when running the test.
     \value Warn A warning generated internally by QtTest
@@ -131,7 +131,9 @@ QT_BEGIN_NAMESPACE
     functions to facilitate testing - such as \l QSignalSpy, \l
     QTestAccessibility, \l QTest::qExtractTestData(), and the facilities to
     deliver artificial mouse and keyboard events - are treated as test code,
-    rather than internal to QtTest.
+    rather than internal to QtTest; they call \l qWarning() and friends rather
+    than using the internal logging infrastructure, so that \l
+    QTest::ignoreMessage() can be used to anticipate the messages.
 
     \sa QAbstractTestLogger::addMessage()
 */
@@ -308,19 +310,21 @@ void QAbstractTestLogger::stopLogging()
 /*!
     \fn void QAbstractTestLogger::addIncident(IncidentTypes type, const char *description, const char *file, int line)
 
-    This virtual method is called when an event occurs that relates to whether
-    the test passes or fails. The \a type indicates whether this was a pass or a
-    fail, whether a failure was expected, and whether the test being run is
-    blacklisted. The \a description may be empty (for a pass), a message
-    describing the failure or, for an expected failure, the explanation of why a
-    failure was expected. Where the location in code of the incident is known,
-    it is indicated by \a file and \a line; otherwise, these are \a nullptr and
-    0, respectively.
+    This virtual method is called when an event occurs that relates to the
+    resolution of the test. The \a type indicates whether this was a pass, a
+    fail or a skip, whether a failure was expected, and whether the test being
+    run is blacklisted. The \a description may be empty (for a pass) or a
+    message describing the nature of the incident. Where the location in code of
+    the incident is known, it is indicated by \a file and \a line; otherwise,
+    these are \a nullptr and 0, respectively.
 
     Every logging implementation must implement this method. Note that there are
     circumstances where more than one incident may be reported, in this way, for
     a single run of a test on a single dataset. It is the implementation's
-    responsibility to recognize such cases and decide what to do about them.
+    responsibility to recognize such cases and decide what to do about them. For
+    purposes of counting resolutions of tests in the "Totals" report at the end
+    of a test run, QtTest considers the first incident (excluding XFail and its
+    blacklisted variant) decisive.
 
     \sa addMessage(), addBenchmarkResult()
 */
@@ -340,17 +344,17 @@ void QAbstractTestLogger::stopLogging()
     \fn void QAbstractTestLogger::addMessage(MessageTypes type, const QString &message, const char *file, int line)
 
     This virtual method is called, via its \c QtMsgType overload, from the
-    custom message handler QtTest installs. It is also used by the
-    implementations of QSKIP(), to warn about various situations detected by
-    QtTest itself, such as \e failure to see a message anticipated by
-    QTest::ignoreMessage() and, particularly when verbosity options have been
-    enabled via the command-line, to log some extra information.
+    custom message handler QtTest installs. It is also used to
+    warn about various situations detected by QtTest itself, such
+    as \e failure to see a message anticipated by QTest::ignoreMessage() and,
+    particularly when verbosity options have been enabled via the command-line,
+    to log some extra information.
 
     Every logging implementation must implement this method. The \a type
     indicates the category of message and the \a message is the content to be
     reported. When the message is associated with specific code, the name of the
-    \a file and \a line number within it are also supplied (otherwise, these are
-    \nullptr and 0, respectively).
+    \a file and \a line number within it are also supplied; otherwise, these are
+    \nullptr and 0, respectively.
 
     \sa QTest::ignoreMessage(), addIncident()
 */
diff --git a/src/testlib/qabstracttestlogger_p.h b/src/testlib/qabstracttestlogger_p.h
index c266e5bdda..fa23484055 100644
--- a/src/testlib/qabstracttestlogger_p.h
+++ b/src/testlib/qabstracttestlogger_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.
@@ -65,6 +65,7 @@ class Q_TESTLIB_EXPORT QAbstractTestLogger
 {
 public:
     enum IncidentTypes {
+        Skip,
         Pass,
         XFail,
         Fail,
@@ -76,14 +77,14 @@ public:
     };
 
     enum MessageTypes {
-        Warn,
-        QWarning,
         QDebug,
+        QInfo,
+        QWarning,
         QCritical,
         QFatal,
-        Skip,
+        // testlib's internal messages:
         Info,
-        QInfo
+        Warn
     };
 
     QAbstractTestLogger(const char *filename);
diff --git a/src/testlib/qappletestlogger.cpp b/src/testlib/qappletestlogger.cpp
index 95798c5361..8764e84aed 100644
--- a/src/testlib/qappletestlogger.cpp
+++ b/src/testlib/qappletestlogger.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2018 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.
@@ -103,6 +103,8 @@ void QAppleTestLogger::addIncident(IncidentTypes type, const char *description,
 {
     MessageData messageData = [=]() {
         switch (type) {
+        case QAbstractTestLogger::Skip:
+            return MessageData{QtInfoMsg, "skip"};
         case QAbstractTestLogger::Pass:
             return MessageData{QtInfoMsg, "pass"};
         case QAbstractTestLogger::XFail:
@@ -153,8 +155,6 @@ void QAppleTestLogger::addMessage(MessageTypes type, const QString &message, con
             return MessageData{QtWarningMsg, "critical"};
         case QAbstractTestLogger::QFatal:
             return MessageData{QtFatalMsg, nullptr};
-        case QAbstractTestLogger::Skip:
-            return MessageData{QtInfoMsg, "skip"};
         case QAbstractTestLogger::Info:
         case QAbstractTestLogger::QInfo:
             return MessageData{QtInfoMsg, nullptr};
@@ -166,16 +166,8 @@ void QAppleTestLogger::addMessage(MessageTypes type, const QString &message, con
     messageData.generateCategory(&category);
 
     QMessageLogContext context(file, line, /* function = */ nullptr, category.data());
-    QString msg = message;
 
-    if (type == Skip) {
-        if (!message.isNull())
-            msg.prepend(testIdentifier() + QLatin1Char('\n'));
-        else
-            msg = testIdentifier();
-    }
-
-    AppleUnifiedLogger::messageHandler(messageData.messageType, context, msg, subsystem());
+    AppleUnifiedLogger::messageHandler(messageData.messageType, context, message, subsystem());
 }
 
 QString QAppleTestLogger::subsystem() const
diff --git a/src/testlib/qjunittestlogger.cpp b/src/testlib/qjunittestlogger.cpp
index 9352cc2180..34cb8b9a75 100644
--- a/src/testlib/qjunittestlogger.cpp
+++ b/src/testlib/qjunittestlogger.cpp
@@ -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.
@@ -238,6 +238,10 @@ void QJUnitTestLogger::addIncident(IncidentTypes type, const char *description,
         // Since XFAIL does not add a failure to the testlog in JUnit XML we add a
         // message, so we still have some information about the expected failure.
         addMessage(QAbstractTestLogger::Info, QString::fromUtf8(description), file, line);
+    } else if (type == QAbstractTestLogger::Skip) {
+        auto skippedElement = new QTestElement(QTest::LET_Skipped);
+        skippedElement->addAttribute(QTest::AI_Message, description);
+        currentTestCase->addChild(skippedElement);
     }
 }
 
@@ -283,12 +287,7 @@ void QJUnitTestLogger::addMessage(MessageTypes type, const QString &message, con
     Q_UNUSED(file);
     Q_UNUSED(line);
 
-    if (type == QAbstractTestLogger::Skip) {
-        auto skippedElement = new QTestElement(QTest::LET_Skipped);
-        skippedElement->addAttribute(QTest::AI_Message, message.toUtf8().constData());
-        currentTestCase->addChild(skippedElement);
-        return;
-    } else if (type == QAbstractTestLogger::QFatal) {
+    if (type == QAbstractTestLogger::QFatal) {
         addFailure(QTest::LET_Error, "qfatal", message);
         return;
     }
diff --git a/src/testlib/qplaintestlogger.cpp b/src/testlib/qplaintestlogger.cpp
index 463fae0e34..75517ac060 100644
--- a/src/testlib/qplaintestlogger.cpp
+++ b/src/testlib/qplaintestlogger.cpp
@@ -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.
@@ -77,6 +77,8 @@ namespace QTest {
     static const char *incidentType2String(QAbstractTestLogger::IncidentTypes type)
     {
         switch (type) {
+        case QAbstractTestLogger::Skip:
+            return "SKIP   ";
         case QAbstractTestLogger::Pass:
             return "PASS   ";
         case QAbstractTestLogger::XFail:
@@ -105,22 +107,20 @@ namespace QTest {
     static const char *messageType2String(QAbstractTestLogger::MessageTypes type)
     {
         switch (type) {
-        case QAbstractTestLogger::Skip:
-            return "SKIP   ";
-        case QAbstractTestLogger::Warn:
-            return "WARNING";
-        case QAbstractTestLogger::QWarning:
-            return "QWARN  ";
         case QAbstractTestLogger::QDebug:
             return "QDEBUG ";
         case QAbstractTestLogger::QInfo:
             return "QINFO  ";
+        case QAbstractTestLogger::QWarning:
+            return "QWARN  ";
         case QAbstractTestLogger::QCritical:
             return "QCRITICAL";
         case QAbstractTestLogger::QFatal:
             return "QFATAL ";
         case QAbstractTestLogger::Info:
             return "INFO   ";
+        case QAbstractTestLogger::Warn:
+            return "WARNING";
         }
         return "??????";
     }
diff --git a/src/testlib/qtaptestlogger.cpp b/src/testlib/qtaptestlogger.cpp
index 8f254a431f..840624fa3b 100644
--- a/src/testlib/qtaptestlogger.cpp
+++ b/src/testlib/qtaptestlogger.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2018 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.
@@ -121,7 +121,8 @@ void QTapTestLogger::addIncident(IncidentTypes type, const char *description,
         return;
     }
 
-    bool ok = type == Pass || type == XPass || type == BlacklistedPass || type == BlacklistedXPass;
+    bool ok = type == Pass || type == BlacklistedPass || type == Skip
+        || type == XPass || type == BlacklistedXPass;
 
     QTestCharBuffer directive;
     if (type == XFail || type == XPass || type == BlacklistedFail || type == BlacklistedPass
@@ -129,6 +130,8 @@ void QTapTestLogger::addIncident(IncidentTypes type, const char *description,
         // We treat expected or blacklisted failures/passes as TODO-failures/passes,
         // which should be treated as soft issues by consumers. Not all do though :/
         QTest::qt_asprintf(&directive, " # TODO %s", description);
+    } else if (type == Skip) {
+        QTest::qt_asprintf(&directive, " # SKIP %s", description);
     }
 
     int testNumber = QTestLog::totalCount();
@@ -246,13 +249,7 @@ void QTapTestLogger::addMessage(MessageTypes type, const QString &message,
 {
     Q_UNUSED(file);
     Q_UNUSED(line);
-
-    if (type == Skip) {
-        QTestCharBuffer directive;
-        QTest::qt_asprintf(&directive, " # SKIP %s", message.toUtf8().constData());
-        outputTestLine(/* ok  = */ true, QTestLog::totalCount(), directive);
-        return;
-    }
+    Q_UNUSED(type);
 
     QTestCharBuffer diagnostics;
     QTest::qt_asprintf(&diagnostics, "# %s\n", qPrintable(message));
diff --git a/src/testlib/qteamcitylogger.cpp b/src/testlib/qteamcitylogger.cpp
index 5ee68af9ad..10d91be178 100644
--- a/src/testlib/qteamcitylogger.cpp
+++ b/src/testlib/qteamcitylogger.cpp
@@ -1,6 +1,7 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 Borgar Ovsthus
+** Copyright (C) 2021 The Qt Company Ltd.
+** Copyright (C) 2017 Borgar Ovsthus
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtTest module of the Qt Toolkit.
@@ -54,6 +55,8 @@ namespace QTest {
     static const char *incidentType2String(QAbstractTestLogger::IncidentTypes type)
     {
         switch (type) {
+        case QAbstractTestLogger::Skip:
+            return "SKIP";
         case QAbstractTestLogger::Pass:
             return "PASS";
         case QAbstractTestLogger::XFail:
@@ -77,22 +80,20 @@ namespace QTest {
     static const char *messageType2String(QAbstractTestLogger::MessageTypes type)
     {
         switch (type) {
-        case QAbstractTestLogger::Skip:
-            return "SKIP";
-        case QAbstractTestLogger::Warn:
-            return "WARNING";
-        case QAbstractTestLogger::QWarning:
-            return "QWARN";
         case QAbstractTestLogger::QDebug:
             return "QDEBUG";
         case QAbstractTestLogger::QInfo:
             return "QINFO";
+        case QAbstractTestLogger::QWarning:
+            return "QWARN";
         case QAbstractTestLogger::QCritical:
             return "QCRITICAL";
         case QAbstractTestLogger::QFatal:
             return "QFATAL";
         case QAbstractTestLogger::Info:
             return "INFO";
+        case QAbstractTestLogger::Warn:
+            return "WARNING";
         }
         return "??????";
     }
@@ -173,6 +174,14 @@ void QTeamCityLogger::addIncident(IncidentTypes type, const char *description,
                              flowID);
 
         outputString(qPrintable(buf));
+    } else if (type == QAbstractTestLogger::Skip) {
+        if (file)
+            detailedText.append(QLatin1String(" |[Loc: %1(%2)|]").arg(QString::fromUtf8(file)).arg(line));
+
+        buf = QLatin1String("##teamcity[testIgnored name='%1' message='%2' flowId='%3']\n")
+                .arg(escapedTestFuncName(), detailedText, flowID);
+
+        outputString(qPrintable(buf));
     }
 
     if (!pendingMessages.isEmpty()) {
@@ -201,21 +210,7 @@ void QTeamCityLogger::addMessage(MessageTypes type, const QString &message,
         return;
 
     QString escapedMessage = tcEscapedString(message);
-
-    QString buf;
-
-    if (type == QAbstractTestLogger::Skip) {
-        if (file)
-            escapedMessage.append(QString(QLatin1String(" |[Loc: %1(%2)|]")).arg(QString::fromUtf8(file)).arg(line));
-
-        buf = QString(QLatin1String("##teamcity[testIgnored name='%1' message='%2' flowId='%3']\n"))
-                .arg(escapedTestFuncName(), escapedMessage, flowID);
-
-        outputString(qPrintable(buf));
-    }
-    else {
-        addPendingMessage(QTest::messageType2String(type), escapedMessage, file, line);
-    }
+    addPendingMessage(QTest::messageType2String(type), escapedMessage, file, line);
 }
 
 QString QTeamCityLogger::tcEscapedString(const QString &str) const
diff --git a/src/testlib/qtestlog.cpp b/src/testlib/qtestlog.cpp
index 7b13f01e24..c90850e786 100644
--- a/src/testlib/qtestlog.cpp
+++ b/src/testlib/qtestlog.cpp
@@ -440,7 +440,7 @@ void QTestLog::addSkip(const char *msg, const char *file, int line)
     ++QTest::skips;
 
     FOREACH_TEST_LOGGER
-        logger->addMessage(QAbstractTestLogger::Skip, QString::fromUtf8(msg), file, line);
+        logger->addIncident(QAbstractTestLogger::Skip, msg, file, line);
 }
 
 void QTestLog::addBenchmarkResult(const QBenchmarkResult &result)
diff --git a/src/testlib/qxmltestlogger.cpp b/src/testlib/qxmltestlogger.cpp
index 0b63b8b753..33bbf7c250 100644
--- a/src/testlib/qxmltestlogger.cpp
+++ b/src/testlib/qxmltestlogger.cpp
@@ -56,22 +56,20 @@ namespace QTest {
     static const char *xmlMessageType2String(QAbstractTestLogger::MessageTypes type)
     {
         switch (type) {
-        case QAbstractTestLogger::Warn:
-            return "warn";
-        case QAbstractTestLogger::QCritical:
-            return "qcritical";
         case QAbstractTestLogger::QDebug:
             return "qdebug";
         case QAbstractTestLogger::QInfo:
             return "qinfo";
         case QAbstractTestLogger::QWarning:
             return "qwarn";
+        case QAbstractTestLogger::QCritical:
+            return "qcritical";
         case QAbstractTestLogger::QFatal:
             return "qfatal";
-        case QAbstractTestLogger::Skip:
-            return "skip";
         case QAbstractTestLogger::Info:
             return "info";
+        case QAbstractTestLogger::Warn:
+            return "warn";
         }
         return "??????";
     }
@@ -79,6 +77,8 @@ namespace QTest {
     static const char *xmlIncidentType2String(QAbstractTestLogger::IncidentTypes type)
     {
         switch (type) {
+        case QAbstractTestLogger::Skip:
+            return "skip";
         case QAbstractTestLogger::Pass:
             return "pass";
         case QAbstractTestLogger::XFail: