QtTest driver v2 #142

konserw · 2017-03-30T11:57:27Z

QtTest based test driver for cucumber-cpp.
Includes also QtTestSteps for both Calc and CalcQt examples.
It is based of PR #140 for Qt in CI
Please take a look at new implementation using pipes for both POSIX and windows.
It doesn't address both your considerations as this is also mostly copied from SO and it's redirecting entire stdout, but for sure it is better than previous implementation. If you still don't like it I'll prepare temporary-file based version.
Only issue I've found is #141 which is here visible as Qt 5.7+ requires C++11.

muggenhor · 2017-03-31T09:45:02Z

CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 2.8.12)
+cmake_minimum_required(VERSION 3.1)


Good catch, I think we already needed 3.1 anyway (for both find_dependency() and Threads::Threads).

muggenhor · 2017-03-31T09:45:38Z

.travis.yml


 before_install:
-  - if [[ "${TRAVIS_OS_NAME}" = "osx" ]]; then brew update && brew install ninja; fi
+  - if [[ "${TRAVIS_OS_NAME}" = "osx" ]]; then brew update && brew install ninja && brew install qt5; fi


Question: doesn't brew support installing multiple packages at once? E.g. brew install ninja qt5 ?

You are probably right - I have no expirience with OSX

muggenhor · 2017-03-31T09:46:41Z

CMakeLists.txt

@@ -13,7 +13,9 @@ set(CUKE_ENABLE_EXAMPLES OFF CACHE BOOL "Enable the examples")
 set(GMOCK_SRC_DIR "" CACHE STRING "Google Mock framework sources path (otherwise downloaded)")
 set(GMOCK_VER "1.7.0" CACHE STRING "Google Mock framework version to be used")
 option(VALGRIND_TESTS "Run tests within Valgrind" OFF)
+set(CUKE_DISABLE_QT OFF CACHE BOOL "Disable using Qt framework")


For boolean parameters I recommend using option instead (it's intended to give user options and only supports booleans).

muggenhor · 2017-03-31T09:47:50Z

CMakeLists.txt

+        execute_process(COMMAND ${QMAKE_EXECUTABLE} -query QT_VERSION OUTPUT_VARIABLE QT_VERSION)
+        if(QT_VERSION GREATER 5.6.99)
+            message(STATUS "C++11 is needed from Qt version 5.7.0, building with c++11 enabled")
+            set(CMAKE_CXX_STANDARD 11)


Why don't you just build the Qt specific bits with C++11 instead of everything?

Setting this for everything fixed TCPserver tests

Are they broken? Because they seem to be running fine on Travis.

AFAIK we're doing a lot of work to be C++98 compatible. Just flipping the switch like this throws away our testing of that.

Please see issue #141 - we can discuss further there the ServerTests.

muggenhor · 2017-03-31T09:49:21Z

CMakeLists.txt

+        #check if c++11 is nedded
+        find_program(QMAKE_EXECUTABLE NAMES qmake HINTS ${QTDIR} ENV QTDIR PATH_SUFFIXES bin)
+        execute_process(COMMAND ${QMAKE_EXECUTABLE} -query QT_VERSION OUTPUT_VARIABLE QT_VERSION)
+        if(QT_VERSION GREATER 5.6.99)


Please use this instead:

if(NOT QT_VERSION VERSION_LESS 5.7)

That does a proper lexicographic comparison of the version number.

muggenhor · 2017-03-31T09:59:00Z

include/cucumber-cpp/internal/utils/qtCapture.hpp

+        // make stdout & stderr streams unbuffered
+        // so that we don't need to flush the streams
+        // before capture and after capture 
+        // (fflush can cause a deadlock if the stream is currently being 


I'm wondering how this sentence should end.

muggenhor · 2017-03-31T10:00:30Z

include/cucumber-cpp/internal/utils/qtCapture.hpp

+        if (m_capturing)
+            return;
+
+        secure_pipe(m_pipe);


Where do all these secure_XXX variants come from? I've never encountered them before.

Those are private methods in that class

Ah thanks, I overlooked those.

muggenhor · 2017-03-31T10:02:24Z

include/cucumber-cpp/internal/utils/qtCapture.hpp

+#define STD_ERR_FD (fileno(stderr))
+#endif
+
+class qtCapture {


None of this code is dependent on a template parameter, so it can all be moved out of this header and into a .cpp file.

Do you mean everything or just the methods definitions ?

The method definitions and all the #includes to support them.

muggenhor · 2017-03-31T10:04:14Z

include/cucumber-cpp/internal/utils/qtCapture.hpp

+        return m_capturing;
+    }
+
+    static void EndCapture() {


Why is this entire class a singleton? It seems to me like the restoring of file descriptors that you're doing here is something that you would want to do in a destructor.

I've converted it to normal class

muggenhor · 2017-03-31T10:05:34Z

src/drivers/QtTestDriver.cpp

+    QtTestObject testObject(this);
+
+    qtCapture::Init();
+    qtCapture::BeginCapture();


I'm assuming you're capturing stdout/stderr here because Qt tends to be overly verbose during execution? Is it possible to make Qt write to some alternate log sink instead?

unfortunately only to file or stdout and we need capturing output to get results

muggenhor · 2017-03-31T11:12:49Z

tests/CMakeLists.txt

@@ -3,7 +3,7 @@ function(cuke_add_driver_test TEST_FILE)
    message(STATUS "Adding " ${TEST_NAME})
    add_executable(${TEST_NAME} ${TEST_FILE}.cpp)
    target_link_libraries(${TEST_NAME} cucumber-cpp-nomain ${CUKE_EXTRA_LIBRARIES} ${ARGN})
-    add_test(NAME ${TEST_NAME} COMMAND ${TEST_NAME})
+    add_test(${TEST_NAME} ${TEST_NAME})


I don't understand why you make this change. And I do believe this is causing your test failures when running with Valgrind.

I'll revert it

konserw · 2017-04-01T14:46:10Z

As for the c++ standard: now change to c++11 is triggered by Qt 5.8 in:

mingw x32
msvc x64
OSX / GMock 1.8.0
so only 3 build out of 14 - is it acceptable ?

muggenhor

The qtCapture code looks to be written without a complete understanding of how to use syscalls under Unix. This is a non-trivial subject that's sometimes very tricky to get right.

Additionally some 5 minute searching in Qt's documentation gave me the suggestion that a message handler could possibly be installed instead to capture this output. I suggest to look at that first, before trying to go this route of intercepting things: https://doc.qt.io/qt-4.8/qtglobal.html#qInstallMsgHandler

muggenhor · 2017-04-02T19:12:28Z

src/drivers/QtTestDriver.cpp

+        do {
+             ret = close(fd);
+             fd_blocked = (errno == EINTR);
+             if (fd_blocked)


EINTR indicates that the syscall was interrupted by a syscall and can be retried immediately. There's no point in waiting first. (The same applies to other syscalls where EINTR is returned).

muggenhor · 2017-04-02T19:12:42Z

src/drivers/QtTestDriver.cpp

+        bool fd_blocked = false;
+        do {
+             ret = dup(src);
+             fd_blocked = (errno == EINTR ||  errno == EBUSY);


This is wrong, because errno's content is undefined when the previous syscall succeeded. I.e. in this case errno is only valid when dup's return value == -1.

muggenhor · 2017-04-02T19:13:25Z

src/drivers/QtTestDriver.cpp

+             fd_blocked = (errno == EINTR);
+             if (fd_blocked)
+                QThread::msleep(10);
+        } while (ret < 0);


This causes you to wait indefinitely for errors that aren't checked for explicitly. Usually you want the converse: only retry for a specific set of known error conditions.

muggenhor · 2017-04-02T19:19:19Z

src/drivers/QtTestDriver.cpp

+#include <QtConcurrent/QtConcurrent>
+
+#ifndef STD_OUT_FD
+#define STD_OUT_FD (fileno(stdout))


Please don't use this macro to hide where you're getting this file descriptor from. You're disguising it as a constant while it isn't.

muggenhor · 2017-04-02T19:25:08Z

src/drivers/QtTestDriver.cpp

+        m_oldStdOut = secure_dup(STD_OUT_FD);
+        m_oldStdErr = secure_dup(STD_ERR_FD);
+        secure_dup2(m_pipe[WRITE], STD_OUT_FD);
+        secure_dup2(m_pipe[WRITE], STD_ERR_FD);


This has one rather fatal flaw: it won't work if whatever you write more than the pipe's buffer can hold to either of these file descriptors. At that point it will block and you're process will hang.

konserw · 2017-04-05T19:08:35Z

Additionally some 5 minute searching in Qt's documentation gave me the suggestion that a message handler could possibly be installed instead to capture this output. I suggest to look at that first, before trying to go this route of intercepting things: https://doc.qt.io/qt-4.8/qtglobal.html#qInstallMsgHandler

Unfortunately all QtTestLib does not rely on normal Qt debug output. they use something like this:

void QAbstractTestLogger::outputString(const char *msg)
{
    QTEST_ASSERT(QTest::stream);

    ::fputs(msg, QTest::stream);
    ::fflush(QTest::stream);
}

Where QtTest::stream if you do not want output to file is:

    if (!out) {
        QTest::stream = stdout;
        return;
}

konserw · 2017-04-05T19:41:34Z

Please reevaluate temporary file based solution as qInstallHandler won't do the trick, and pipe based solution wasn't as good as I believed.

muggenhor

This looks much better. I've got just a few comments and questions now.

muggenhor · 2017-04-12T10:01:49Z

src/drivers/QtTestDriver.cpp

-    int returnValue = QTest::qExec(&testObject, 0, NULL);
-    capture.EndCapture();
+    QTemporaryFile file;
+    QString fileName;


Please remove this unused variable

muggenhor · 2017-04-12T10:06:10Z

src/drivers/QtTestDriver.cpp

+    if(!file.open()) {
+        return InvokeResult::failure("Unable to open temporary file needed for this test");
+    }
+    file.close();


Doesn't this or reopening it later destroy the used name? I also don't think it's necessary to close. As long as you don't read from it the buffer shouldn't be filled so you shouldn't have any problems of it containing stale data.

It doesn't destroy used name, but it turns out to be unnecessary to close and reopen.

muggenhor · 2017-04-12T10:06:39Z

src/drivers/QtTestDriver.cpp


+    QtTestObject testObject(this);
+    int returnValue = QTest::qExec(&testObject, QStringList() << "test" << "-o" << file.fileName());
    if(returnValue == 0)


Add a space between if and (

muggenhor · 2017-04-12T10:08:45Z

src/drivers/QtTestDriver.cpp

+    {
+        file.open();
+        QTextStream ts(&file);
+        return InvokeResult::failure(ts.readAll().toLocal8Bit());


We're converting this to JSON later on. This should definitely be encoded to UTF-8, not to the local character set. Let cucumber take care of doing that conversion when it wishes to display its output.

konserw · 2017-08-23T20:42:12Z

Closing this as I've created another revision of this PR #165.

konserw added 2 commits March 30, 2017 11:55

Rework CMake logic around Qt and use Qt in CI

964af78

QtTestDriver + tests and examples

26e6feb

konserw mentioned this pull request Mar 30, 2017

QtTest driver #110

Closed

Run qttest calculator example in travis

411cdbd

konserw mentioned this pull request Mar 30, 2017

Flaky TCPSocketServer unit test #141

Closed

konserw added 3 commits March 30, 2017 14:38

Pipe fix for mingw

d833723

fix setting qt5_found variable

b1eaf2c

If c++11 is needed enable for all

795ced6

muggenhor requested changes Mar 31, 2017

View reviewed changes

konserw added 2 commits March 31, 2017 16:59

First review

15164eb

Update CMake verision requirement

d1b3ffd

konserw mentioned this pull request Mar 31, 2017

Enable Qt framework in CI #140

Closed

konserw added 5 commits March 31, 2017 22:11

removed submodule added by mistake

5b10e66

Merge branch 'cirework' into qttest2

3218931

Most of frirst review

0202bed

convert qtCapture to normal class

ff88570

revert change to add_test

0858492

muggenhor requested changes Apr 2, 2017

View reviewed changes

Temporary file based solution

e2373da

muggenhor requested changes Apr 12, 2017

View reviewed changes

konserw added 2 commits April 13, 2017 11:35

Second review

288a6ad

examples

742b88d

muggenhor mentioned this pull request Jun 3, 2017

Design a plugin(-like) architecture for test drivers #154

Open

konserw closed this Aug 23, 2017

konserw deleted the qttest2 branch April 2, 2018 10:06

		@@ -1,4 +1,4 @@
		cmake_minimum_required(VERSION 2.8.12)
		cmake_minimum_required(VERSION 3.1)

QtTest driver v2 #142

QtTest driver v2 #142

Conversation

konserw commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Apr 1, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Apr 5, 2017

konserw commented Apr 5, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Aug 23, 2017

muggenhor Mar 31, 2017 •

edited

Loading