QtTest driver v3 #165

konserw · 2017-08-23T20:41:06Z

Summary

QtTest based test driver for cucumber-cpp.

Details

Implementation based on temporary file.
When building QtTest driver C++11 is enabled globally to prevent issue #141.

Motivation and Context

It's useful for projects written in Qt.
It is third revision of this driver.

How Has This Been Tested?

Unit test is included as well as steps for examples.

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

It is my own work, its copyright is implicitly assigned to the project and no substantial part of it has been copied from other sources (including Stack Overflow). In rare occasions this is acceptable, like in CMake modules where the original copyright information should be kept.
I'm using the same code standards as the existing code (indentation, spacing, variable naming, ...).
I've added tests for my code.
I have verified whether my change requires changes to the documentation
My change either requires no documentation change or I've updated the documentation accordingly.
My branch has been rebased to master, keeping only relevant commits.

muggenhor

Just a quick scan through the source, definitely didn't look at everything yet.

The failing tests on Travis are a bit worrying.

muggenhor · 2017-08-24T10:03:35Z

CMakeLists.txt

@@ -122,8 +122,13 @@ if(NOT CUKE_DISABLE_QT)
    find_package(Qt5Test)

    if(${Qt5Core_FOUND} AND ${Qt5Widgets_FOUND} AND ${Qt5Test_FOUND})
+        set(QT5_FOUND true)


Please just check for Qt5TEST_FOUND instead where you need it.

muggenhor · 2017-08-24T10:09:03Z

CMakeLists.txt

        message(STATUS "Found Qt version: ${Qt5Core_VERSION_STRING}")
        set(QT_LIBRARIES Qt5::Core Qt5::Widgets Qt5::Test)
+        if(NOT ${Qt5Core_VERSION_STRING} VERSION_LESS 5.7)
+            message(STATUS "C++11 is needed from Qt version 5.7.0, building with c++11 enabled")
+            set(CMAKE_CXX_STANDARD 11)


This precludes building as C++14 or 17. You could instead use target_compile_features(cucumber-cpp PUBLIC ${some_CXX11_features_that_Qt_5_7_uses}) where that list of features that you require are a subset of CMAKE_CXX_KNOWN_FEATURES.

Or an easier approach may be to write it like this to prevent overriding the user-requested C++ version:

if(NOT Qt5Core_VERSION_STRING VERSION_LESS 5.7 AND (NOT DEFINED CMAKE_CXX_STANDARD OR NOT CMAKE_CXX_STANDARD STREQUAL 98))

I'll go for the second version :)

muggenhor · 2017-08-24T10:10:33Z

examples/CalcQt/CMakeLists.txt

+    if(QT5_FOUND)
+        add_executable(QtTestCalculatorQtSteps features/step_definitions/QtTestCalculatorQtSteps)
+        set_target_properties(QtTestCalculatorQtSteps PROPERTIES AUTOMOC ON)
+        target_include_directories(QtTestCalculatorQtSteps PUBLIC src)


Make that PRIVATE instead. PUBLIC is generally only useful for libraries.

copy-paste issue :)

muggenhor · 2017-08-24T10:10:52Z

examples/CalcQt/CMakeLists.txt

+        add_executable(QtTestCalculatorQtSteps features/step_definitions/QtTestCalculatorQtSteps)
+        set_target_properties(QtTestCalculatorQtSteps PROPERTIES AUTOMOC ON)
+        target_include_directories(QtTestCalculatorQtSteps PUBLIC src)
+        target_link_libraries(QtTestCalculatorQtSteps libcalcqt ${QT_LIBRARIES} ${CUKE_LIBRARIES})


Please add PRIVATE between the target and the libraries it links to.

ok - didn't know it is necessary

muggenhor · 2017-08-24T10:12:47Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+    Q_OBJECT
+public:
+    QtTestObject(QtTestStep* qtTestStep): step(qtTestStep) {}
+    virtual ~QtTestObject() {}


You're requiring C++11 anyway, correct?

Then please use

virtual ~QtTestObject() = default;

only for Qt > 5.7, which is not the case for instance on our travis linux instances

Ack, thanks for clearing this up.

muggenhor · 2017-08-24T15:04:49Z

examples/CalcQt/CMakeLists.txt

@@ -12,8 +12,8 @@ if(QT_LIBRARIES)
    if(Qt5TEST_FOUND)
        add_executable(QtTestCalculatorQtSteps features/step_definitions/QtTestCalculatorQtSteps)
        set_target_properties(QtTestCalculatorQtSteps PROPERTIES AUTOMOC ON)
-        target_include_directories(QtTestCalculatorQtSteps PUBLIC src)
-        target_link_libraries(QtTestCalculatorQtSteps libcalcqt ${QT_LIBRARIES} ${CUKE_LIBRARIES})
+        target_include_directories(QtTestCalculatorQtSteps PRIVATE src)


Looking at this again. You're probably adding this for the examples/CalcQt/src/CalculatorWidget.h file. But that's already reachable because you're linking to libcalcqt. I.e. CMake propagates the usage requirement of libcalcqt to this target. You shouldn't need this entire target_include_directories directive.

konserw · 2017-08-24T19:25:20Z

@muggenhor - any idea why first appveyor build failed?

konserw · 2017-08-25T12:55:03Z

All green at last! And rebased onto master + squashed all commits. @muggenhor please re-review :)

muggenhor

Looks really well done, just one CMake remark and a few nitpicks.

muggenhor · 2017-08-25T14:37:22Z

src/CMakeLists.txt

+    qt5_wrap_cpp(MOC_FILE ../include/cucumber-cpp/internal/drivers/QtTestDriver.hpp)
+    list(APPEND CUKE_SOURCES ${MOC_FILE})
+    list(APPEND CUKE_SOURCES drivers/QtTestDriver.cpp)
+    list(APPEND CUKE_DEP_LIBRARIES ${QT_LIBRARIES})


Please only add Qt5::Test, the rest isn't necessary.

muggenhor · 2017-08-25T14:38:30Z

src/drivers/QtTestDriver.cpp

+
+const InvokeResult QtTestStep::invokeStepBody() {
+    QTemporaryFile file;
+    QString fileName;


Unused variable?

Forgot about it :)

muggenhor · 2017-08-25T14:40:12Z

src/drivers/QtTestDriver.cpp

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


Space between if and opening brace please.

ok, we need that clang-format PR merged :)

You can copy the .clang-format file from that PR into your local working copy and execute this to format everything changes since master (replace origin/master with the most recent commit of the master branch that you based your changes on).:

git clang-format-3.8 --binary=/usr/bin/clang-format-3.8 --style=file --commit=origin/master

muggenhor · 2017-08-25T14:40:55Z

tests/integration/drivers/QtTestDriverTest.cpp

+class QtTestStepDouble : public QtTestStep {
+public:
+    QtTestStepDouble() : QtTestStep() {
+        testRun = false;


Initialize this member var in the initializer list.

muggenhor · 2017-08-25T14:42:32Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+    Q_OBJECT
+public:
+    QtTestObject(QtTestStep* qtTestStep): step(qtTestStep) {}
+    virtual ~QtTestObject() {}


Ack, thanks for clearing this up.

muggenhor

Some clang-format inspired changes

muggenhor · 2017-08-25T15:15:55Z

examples/Calc/features/step_definitions/QtTestCalculatorSteps.cpp

@@ -0,0 +1,4 @@
+#include <QTest>
+// Pretend to be GTest
+#define EXPECT_EQ QCOMPARE 


Please remove whitespace from the end of the line.

muggenhor · 2017-08-25T15:16:28Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+namespace cucumber {
+namespace internal {
+
+class QtTestStep : public BasicStep{


need a space between the base class and opening curly brace

muggenhor · 2017-08-25T15:16:41Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+
+class QtTestStep : public BasicStep{
+    friend class QtTestObject;
+public:


need an empty line before the access label

muggenhor · 2017-08-25T15:16:54Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+class QtTestStep : public BasicStep{
+    friend class QtTestObject;
+public:
+    QtTestStep(): BasicStep() {}


space between braces and colon

muggenhor · 2017-08-25T15:17:05Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+
+#define STEP_INHERITANCE(step_name) ::cucumber::internal::QtTestStep
+
+class QtTestObject: public QObject {


space before colon

muggenhor · 2017-08-25T15:17:14Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+class QtTestObject: public QObject {
+    Q_OBJECT
+public:
+    QtTestObject(QtTestStep* qtTestStep): step(qtTestStep) {}


space before colon

muggenhor · 2017-08-25T15:18:11Z

src/drivers/QtTestDriver.cpp

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


Please put curly braces around this statement (using different styles for the if and else branch is inconsistent).

muggenhor · 2017-08-25T15:18:56Z

src/drivers/QtTestDriver.cpp

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


clang-format suggests this instead, and I like it.

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

muggenhor · 2017-08-25T15:19:26Z

tests/integration/drivers/QtTestDriverTest.cpp

+    QtTestDriverTest test;
+    return test.run();
+}
+


Please no empty line at end of file

muggenhor · 2017-08-25T15:20:35Z

tests/integration/drivers/QtTestDriverTest.cpp

+
+class QtTestStepDouble : public QtTestStep {
+public:
+    QtTestStepDouble() : QtTestStep(), testRun(false) { }


No space between curly braces

konserw · 2017-08-25T20:40:02Z

I've applied all those format changes, I'll rebase/squash now

muggenhor · 2017-08-26T13:19:30Z

examples/Calc/CMakeLists.txt

+
+if(Qt5TEST_FOUND)
+    add_executable(QtTestCalculatorSteps features/step_definitions/QtTestCalculatorSteps)
+    target_link_libraries(QtTestCalculatorSteps Calc ${QT_LIBRARIES} ${CUKE_LIBRARIES})


I think you can replace ${QT_LIBRARIES} with Qt5::Test here as you only need the test framework, not the GUI stuff.

muggenhor · 2017-08-26T13:21:55Z

examples/CalcQt/CMakeLists.txt

@@ -9,12 +9,20 @@ if(QT_LIBRARIES)
    add_executable(calcqt src/CalcQt.cpp)
    target_link_libraries(calcqt libcalcqt ${QT_LIBRARIES})

+    if(Qt5TEST_FOUND)
+        add_executable(QtTestCalculatorQtSteps features/step_definitions/QtTestCalculatorQtSteps)
+        set_target_properties(QtTestCalculatorQtSteps PROPERTIES AUTOMOC ON)


Why do you enable AUTOMOC for this? I don't see any moc-able object in your one source file.

muggenhor · 2017-08-26T13:22:54Z

include/cucumber-cpp/internal/drivers/QtTestDriver.hpp

+class QtTestObject : public QObject {
+    Q_OBJECT
+public:
+    QtTestObject(QtTestStep* qtTestStep): step(qtTestStep) {}


missing space before colon

coveralls · 2017-08-31T19:48:49Z

Coverage decreased (-0.01%) to 96.7% when pulling 1d34255 on konserw:qttest3 into 6551a0e on cucumber:master.

konserw · 2017-09-13T11:26:22Z

Squashed and rebased - I think it's ready for merge. @muggenhor do you agree?

konserw · 2017-09-13T21:41:10Z

Build is failing due to #169

coveralls · 2017-10-08T16:25:10Z

Coverage decreased (-0.003%) to 96.697% when pulling 89d5080 on konserw:qttest3 into 0928f25 on cucumber:master.

Implementation based on temporary file. When building QtTest driver C++11 is enabled globally to prevent issue cucumber#141.

coveralls · 2017-10-12T21:00:27Z

Coverage remained the same at 96.707% when pulling fe975ee on konserw:qttest3 into ec26000 on cucumber:master.

konserw · 2017-10-13T08:49:07Z

Ok, rebased and it's all green now. Any more comments or shall I merge?

marco-piccolino · 2017-11-03T08:27:19Z

hi!
Is this gonna make it into master any soon?
Really looking forward to it.
Thanks for the hard work!

paoloambrosio · 2017-11-04T14:03:36Z

@konserw great contribution!

muggenhor · 2017-11-09T13:52:45Z

Nice work @konserw !

konserw mentioned this pull request Aug 23, 2017

QtTest driver v2 #142

Closed

konserw requested a review from muggenhor August 23, 2017 20:42

konserw added the feature label Aug 23, 2017

muggenhor requested changes Aug 24, 2017

View reviewed changes

muggenhor reviewed Aug 24, 2017

View reviewed changes

konserw force-pushed the qttest3 branch from e843aae to b966ed6 Compare August 25, 2017 10:15

muggenhor requested changes Aug 25, 2017

View reviewed changes

cucumber deleted a comment from coveralls Aug 25, 2017

konserw force-pushed the qttest3 branch from ff90d35 to d52b98a Compare August 25, 2017 21:18

cucumber deleted a comment from coveralls Aug 25, 2017

muggenhor requested changes Aug 26, 2017

View reviewed changes

cucumber deleted a comment from coveralls Sep 1, 2017

muggenhor approved these changes Sep 3, 2017

View reviewed changes

konserw force-pushed the qttest3 branch from 1d34255 to 89d5080 Compare September 13, 2017 11:25

konserw mentioned this pull request Oct 8, 2017

BoostCalcQt segfaults on travis #169

Closed

konserw added 2 commits October 12, 2017 19:56

[QtTestDriver] Qt test driver for cucumber-cpp.

e83d8bb

Implementation based on temporary file. When building QtTest driver C++11 is enabled globally to prevent issue cucumber#141.

[QtTestDriver] Unit test

1c9f5f2

konserw added 2 commits October 12, 2017 19:56

[QtTestDriver] QtTest steps for examples

3895c55

[QtTestDriver] Documentation

fe975ee

konserw force-pushed the qttest3 branch from 89d5080 to fe975ee Compare October 12, 2017 18:05

muggenhor approved these changes Nov 2, 2017

View reviewed changes

paoloambrosio merged commit fe975ee into cucumber:master Nov 4, 2017

paoloambrosio added a commit that referenced this pull request Nov 4, 2017

Merge #165 'QtTest driver'

38a4b34

konserw deleted the qttest3 branch April 2, 2018 10:07

muggenhor mentioned this pull request May 19, 2018

QtTestDriverTest doesn't get built nor executed and leaks #190

Closed


		#define STEP_INHERITANCE(step_name) ::cucumber::internal::QtTestStep

		class QtTestObject: public QObject {

QtTest driver v3 #165

QtTest driver v3 #165

Conversation

konserw commented Aug 23, 2017

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

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 24, 2017

konserw commented Aug 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 31, 2017

konserw commented Sep 13, 2017

konserw commented Sep 13, 2017

coveralls commented Oct 8, 2017

coveralls commented Oct 12, 2017

konserw commented Oct 13, 2017

marco-piccolino commented Nov 3, 2017

paoloambrosio commented Nov 4, 2017

muggenhor commented Nov 9, 2017