testlib: Prevent Apple test logger from stomping on other loggers

We were potentially adding the Apple test logger multiple times, and we
didn't consider whether the existing loggers were logging to file or not
when circumventing them.

We now don't use the Apple logger if it would touch stderr and some other
logger is using stdout. In the case of no explicit logger being specified
on the command line, we allow the Apple logger to take priority over the
default plain test logger.

Change-Id: I31bbec4f4b3ab84ba9a2be35e8e5db08fee071a7
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

5 files changed, 49 insertions, 43 deletions

diff --git a/src/testlib/qappletestlogger.cpp b/src/testlib/qappletestlogger.cpp
index 8e75da88f8..959ff6cf64 100644
--- a/src/testlib/qappletestlogger.cpp
+++ b/src/testlib/qappletestlogger.cpp
@@ -55,9 +55,8 @@ bool QAppleTestLogger::debugLoggingEnabled()
     return os_log_type_enabled(OS_LOG_DEFAULT, OS_LOG_TYPE_DEBUG);
 }
 
-QAppleTestLogger::QAppleTestLogger(QAbstractTestLogger *logger)
+QAppleTestLogger::QAppleTestLogger()
     : QAbstractTestLogger(nullptr)
-    , m_logger(logger)
 {
 }
 
@@ -65,6 +64,8 @@ static QAppleLogActivity testFunctionActivity;
 
 void QAppleTestLogger::enterTestFunction(const char *function)
 {
+    Q_UNUSED(function);
+
     // Re-create activity each time
     testFunctionActivity = QT_APPLE_LOG_ACTIVITY("Running test function").enter();
 
@@ -73,15 +74,12 @@ void QAppleTestLogger::enterTestFunction(const char *function)
     QString identifier = QString::fromLatin1(testIdentifier.data());
     QMessageLogContext context(nullptr, 0, nullptr, "qt.test.enter");
     QString message = identifier;
-    if (AppleUnifiedLogger::messageHandler(QtDebugMsg, context, message, identifier))
-        return; // AUL already printed to stderr
 
-    m_logger->enterTestFunction(function);
+    AppleUnifiedLogger::messageHandler(QtDebugMsg, context, message, identifier);
 }
 
 void QAppleTestLogger::leaveTestFunction()
 {
-    m_logger->leaveTestFunction();
     testFunctionActivity.leave();
 }
 
@@ -134,18 +132,12 @@ void QAppleTestLogger::addIncident(IncidentTypes type, const char *description,
     if (qstrlen(description))
         message += QLatin1Char('\n') % QString::fromLatin1(description);
 
-    if (AppleUnifiedLogger::messageHandler(incidentClassification.first, context, message, subsystem))
-        return; // AUL already printed to stderr
-
-    m_logger->addIncident(type, description, file, line);
+    AppleUnifiedLogger::messageHandler(incidentClassification.first, context, message, subsystem);
 }
 
 void QAppleTestLogger::addMessage(QtMsgType type, const QMessageLogContext &context, const QString &message)
 {
-    if (AppleUnifiedLogger::messageHandler(type, context, message))
-        return; // AUL already printed to stderr
-
-    m_logger->addMessage(type, context, message);
+    AppleUnifiedLogger::messageHandler(type, context, message);
 }
 
 #endif // QT_USE_APPLE_UNIFIED_LOGGING
diff --git a/src/testlib/qappletestlogger_p.h b/src/testlib/qappletestlogger_p.h
index 5a45fad7a0..4217f4e6a2 100644
--- a/src/testlib/qappletestlogger_p.h
+++ b/src/testlib/qappletestlogger_p.h
@@ -63,12 +63,7 @@ class QAppleTestLogger : public QAbstractTestLogger
 public:
     static bool debugLoggingEnabled();
 
-    QAppleTestLogger(QAbstractTestLogger *logger);
-
-    void startLogging() override
-    { m_logger->startLogging(); }
-    void stopLogging() override
-    { m_logger->stopLogging(); }
+    QAppleTestLogger();
 
     void enterTestFunction(const char *function) override;
     void leaveTestFunction() override;
@@ -77,16 +72,12 @@ public:
                      const char *file = 0, int line = 0) override;
     void addMessage(QtMsgType, const QMessageLogContext &,
             const QString &) override;
-
-    void addBenchmarkResult(const QBenchmarkResult &result) override
-    { m_logger->addBenchmarkResult(result); }
-
     void addMessage(MessageTypes type, const QString &message,
                             const char *file = 0, int line = 0) override
-    { m_logger->addMessage(type, message, file, line); }
+    { Q_UNUSED(type); Q_UNUSED(message); Q_UNUSED(file); Q_UNUSED(line); Q_UNREACHABLE(); }
 
-private:
-    QScopedPointer<QAbstractTestLogger> m_logger;
+    void addBenchmarkResult(const QBenchmarkResult &result) override
+    { Q_UNUSED(result); }
 };
 #endif
 
diff --git a/src/testlib/qtestcase.cpp b/src/testlib/qtestcase.cpp
index 22e8ac49bc..0c935a1f1c 100644
--- a/src/testlib/qtestcase.cpp
+++ b/src/testlib/qtestcase.cpp
@@ -77,6 +77,10 @@
 #include <QtTest/private/qtestutil_macos_p.h>
 #endif
 
+#if defined(Q_OS_DARWIN)
+#include <QtTest/private/qappletestlogger_p.h>
+#endif
+
 #include <cmath>
 #include <numeric>
 #include <algorithm>
@@ -509,7 +513,7 @@ static int qToInt(char *str)
 
 Q_TESTLIB_EXPORT void qtest_qParseArgs(int argc, char *argv[], bool qml)
 {
-    QTestLog::LogMode logFormat = QTestLog::Plain;
+    int logFormat = -1; // Not set
     const char *logFilename = 0;
 
     QTest::testFunctions.clear();
@@ -677,7 +681,7 @@ Q_TESTLIB_EXPORT void qtest_qParseArgs(int argc, char *argv[], bool qml)
                     fprintf(stderr, "only one logger can log to stdout\n");
                     exit(1);
                 }
-                QTestLog::addLogger(logFormat, filename);
+                QTestLog::addLogger(QTestLog::LogMode(logFormat), filename);
             }
             delete [] filename;
             delete [] format;
@@ -839,10 +843,25 @@ Q_TESTLIB_EXPORT void qtest_qParseArgs(int argc, char *argv[], bool qml)
     QTestLog::setInstalledTestCoverage(installedTestCoverage);
 
     // If no loggers were created by the long version of the -o command-line
-    // option, create a logger using whatever filename and format were
-    // set using the old-style command-line options.
-    if (QTestLog::loggerCount() == 0)
-        QTestLog::addLogger(logFormat, logFilename);
+    // option, but a logger was requested via the old-style option, add it.
+    const bool explicitLoggerRequested = logFormat != -1;
+    if (QTestLog::loggerCount() == 0 && explicitLoggerRequested)
+        QTestLog::addLogger(QTestLog::LogMode(logFormat), logFilename);
+
+    bool addFallbackLogger = !explicitLoggerRequested;
+
+#if defined(QT_USE_APPLE_UNIFIED_LOGGING)
+    // Any explicitly requested loggers will be added by now, so we can check if they use stdout
+    const bool safeToAddAppleLogger = !AppleUnifiedLogger::willMirrorToStderr() || !QTestLog::loggerUsingStdout();
+    if (safeToAddAppleLogger && QAppleTestLogger::debugLoggingEnabled()) {
+        QTestLog::addLogger(QTestLog::Apple, nullptr);
+        if (AppleUnifiedLogger::willMirrorToStderr() && !logFilename)
+            addFallbackLogger = false; // Prevent plain test logger fallback below
+    }
+#endif
+
+    if (addFallbackLogger)
+        QTestLog::addLogger(QTestLog::Plain, logFilename);
 }
 
 QBenchmarkResult qMedian(const QVector<QBenchmarkResult> &container)
diff --git a/src/testlib/qtestlog.cpp b/src/testlib/qtestlog.cpp
index 80f905e962..57bb7d95a7 100644
--- a/src/testlib/qtestlog.cpp
+++ b/src/testlib/qtestlog.cpp
@@ -460,6 +460,11 @@ void QTestLog::addLogger(LogMode mode, const char *filename)
     case QTestLog::TAP:
         logger = new QTapTestLogger(filename);
         break;
+#if defined(QT_USE_APPLE_UNIFIED_LOGGING)
+    case QTestLog::Apple:
+        logger = new QAppleTestLogger;
+        break;
+#endif
 #if defined(HAVE_XCTEST)
     case QTestLog::XCTest:
         logger = new QXcodeTestLogger;
@@ -467,14 +472,6 @@ void QTestLog::addLogger(LogMode mode, const char *filename)
 #endif
     }
 
-#if defined(QT_USE_APPLE_UNIFIED_LOGGING)
-    // Logger that also feeds messages to AUL. It needs to wrap the existing
-    // logger, as it needs to be able to short circuit the existing logger
-    // in case AUL prints to stderr.
-    if (QAppleTestLogger::debugLoggingEnabled())
-        logger = new QAppleTestLogger(logger);
-#endif
-
     QTEST_ASSERT(logger);
     QTest::loggers.append(logger);
 }
diff --git a/src/testlib/qtestlog_p.h b/src/testlib/qtestlog_p.h
index 0bdd6290e1..e63e89a78e 100644
--- a/src/testlib/qtestlog_p.h
+++ b/src/testlib/qtestlog_p.h
@@ -53,6 +53,10 @@
 
 #include <QtTest/qttestglobal.h>
 
+#if defined(Q_OS_DARWIN)
+#include <QtCore/private/qcore_mac_p.h>
+#endif
+
 QT_BEGIN_NAMESPACE
 
 class QBenchmarkResult;
@@ -63,9 +67,12 @@ class Q_TESTLIB_EXPORT QTestLog
 {
 public:
     enum LogMode {
-        Plain = 0, XML, LightXML, XunitXML, CSV, TeamCity, TAP,
+        Plain = 0, XML, LightXML, XunitXML, CSV, TeamCity, TAP
+#if defined(QT_USE_APPLE_UNIFIED_LOGGING)
+        , Apple
+#endif
 #if defined(HAVE_XCTEST)
-        XCTest
+        , XCTest
 #endif
     };