Fix issue #81 by intercepting messages from BOOST_*_MESSAGE macros #164

konserw · 2017-08-23T16:22:16Z

Summary

Intercept messages from BOOST_*_MESSAGE macros

Details

Motivation and Context

Fixes #81

How Has This Been Tested?

It has been tested manually by adding following line to BoostCalc example and running that example:
BOOST_CHECK_MESSAGE(false, "Message");

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:

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 · 2017-08-23T17:03:28Z

src/drivers/BoostDriver.cpp

-    void log_entry_value( std::ostream&, const_string /*value*/);
-    void log_entry_value( std::ostream&, lazy_ostream const& /*value*/) {};
+    void log_entry_value( std::ostream&, const_string value);
+    void log_entry_value( std::ostream&, lazy_ostream const& value);


Please don't do this: this causes warnings about "unused parameters", that's the reason it was commented out like that in the first place.

But we are actually using them?

grep for unused and for param hasn't show me any warning

Oh, nevermind, I didn't notice that you changed the empty inline function ({}) to just a declaration with the function's definition elsewhere.

As for warnings: you should compile with -Wall -Wextra to see any. I think we should probably consider building with that one plus -Werror by default on our CI systems at least.

The old code (with empty inline function definition) without commented out parameter produces this with `-DCMAKE_CXX_FLAGS='-Wall -Wextra -Werror':

../src/drivers/BoostDriver.cpp:64:62: error: unused parameter 'value' [-Werror,-Wunused-parameter] void log_entry_value( std::ostream&, lazy_ostream const& value) {}; ^

I thought we are doing so for debug builds, but now I can see that there is only -Weffc++ being set for those. We could add those flags to CMAKE_CXX_FLAGS_DEBUG in CMakelists ?

I've just submitted 3ec9d03 which enables warnings for all GCC/Clang builds. I don't think it makes sense to make it dependent on the build type. Adding -Werror is something we should do only when the warnings are fixed.

muggenhor · 2017-08-23T20:56:36Z

Can you also add a test (perhaps in tests/integration/drivers) that fails without this change and succeeds with it?

konserw · 2017-08-23T21:12:22Z

Gladly, but I'm not sure how

konserw · 2017-08-25T05:57:47Z

@muggenhor - could you take a look at the test I've added? I'm not good with ruby... Also maybe we should rethink how the "support code" is inserted to every feature with C++ code?

muggenhor

I'm not a Ruby expert myself, but they mostly look good to me.

One thing that I'm missing though is a way to prevent this feature from being executed if we're building without Boost.Test. Not sure how easy that would or wouldn't be. Probably it involves adding a tag to the feature and adding a parameter to filter that out in the CMake features* targets.

PS Do you have a link to a travis job demonstrating that this new test case fails without your changes to the driver?

muggenhor · 2017-08-25T14:52:46Z

CMakeLists.txt

@@ -216,7 +216,7 @@ else()
        add_executable(e2e-steps EXCLUDE_FROM_ALL ${CUKE_DYNAMIC_CPP_STEPS})
        # Mark this file as generated so it isn't required at CMake generation time (it is necessary when the target gets built though)
        set_source_files_properties(${CUKE_DYNAMIC_CPP_STEPS} PROPERTIES GENERATED TRUE)
-        target_link_libraries(e2e-steps ${CUKE_LIBRARIES})
+        target_link_libraries(e2e-steps ${CUKE_LIBRARIES} ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})


Make this a separate statement, like this:

if(Boost_UNIT_TEST_FRAMEWORK_FOUND) target_link_libraries(e2e-steps PRIVATE ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY}) endif()

Good point.

muggenhor · 2017-08-25T14:53:49Z

examples/Calc/features/step_definitions/BoostCalculatorSteps.cpp

@@ -1,4 +1,5 @@
 #include <boost/test/unit_test.hpp>
 // Pretend to be GTest
 #define EXPECT_EQ BOOST_CHECK_EQUAL
+#include <cucumber-cpp/autodetect.hpp>


this line should not be necessary as it's the first line found in CalculatorSteps.cpp

muggenhor · 2017-08-25T14:56:52Z

features/step_definitions/cucumber_cpp_steps.rb

@@ -1,3 +1,7 @@
+Given /^a step definition file without support code:$/ do |code|


Please rename to "a step definition file with only this content"

muggenhor · 2017-08-25T14:57:26Z

features/step_definitions/cucumber_cpp_mappings.rb

@@ -228,6 +228,10 @@ def append_step_definition(step_name, code) # todo params parameter?
 EOF
  end

+  def no_support_code(code)


rename to only_support_code

this is without the support code (code from support folder)

muggenhor · 2017-08-25T14:58:04Z

features/specific/predicate_message.feature

@@ -0,0 +1,70 @@
+@wip


Please remove this tag ;-)

or replace with @boost ?

Yes, that's a good one

muggenhor · 2017-08-25T15:00:01Z

features/specific/predicate_message.feature

+
+  This is just a simple feature meant to test the fix for issue #81.
+  Problem was Cucumber-Cpp didn't recoginize failed asserations
+  when using following Boost macros:


Rephrase without the issue please. I.e. something like "Support Boost.Test's different validation and reporting macros..."

konserw · 2017-08-25T19:22:48Z

@muggenhor how do you like it now? I kinda changed it other way around...

konserw · 2017-08-25T20:36:20Z

I've pasted wrong link twice. For failing test see this one: https://travis-ci.org/konserw/cucumber-cpp/jobs/268053073

muggenhor

Looks good for merging with the one suggested change.

muggenhor · 2017-08-26T13:27:31Z

CMakeLists.txt

-        add_feature_target(features --format progress)
-        add_feature_target(features-pretty --format pretty)
-        add_feature_target(features-wip --format pretty --tags @wip)
+        add_feature_target(features         ${CUKE_E2E_TAGS} --format progress)


Just add it before ${ARGN} in the add_feature_target function instead. That reduces repetition.

paoloambrosio

Nicely done! Just some comments on the feature file...

paoloambrosio · 2017-09-05T05:25:13Z

features/specific/predicate_message.feature

@@ -0,0 +1,72 @@
+@boost
+Feature: Predicate Message Feature


Either this should be in a boost directory or it should specify that it is related to Boost. Please remove the redundant Feature at the end.

paoloambrosio · 2017-09-05T05:25:59Z

features/specific/predicate_message.feature

+  This is just a simple feature meant to test boost's 
+  validation macros that also output custom message.
+  Problem was Cucumber-Cpp didn't capture those messages
+  and even didn't recognize failed asserations.


The description should capture the behaviour, not why you did it (you should find that in the PR).

paoloambrosio · 2017-09-05T05:27:04Z

features/specific/predicate_message.feature

+  Expected results when using following Boost macros:
+    BOOST_WARN_MESSAGE( predicate, message ) -> should pass
+    BOOST_CHECK_MESSAGE( predicate, message ) -> should fail
+    BOOST_REQUIRE_MESSAGE( predicate, message ) -> should fail


Redundant and can get out of sync with the scenarios.

paoloambrosio · 2017-09-05T05:29:25Z

features/specific/predicate_message.feature

+    BOOST_CHECK_MESSAGE( predicate, message ) -> should fail
+    BOOST_REQUIRE_MESSAGE( predicate, message ) -> should fail
+
+  Scenario: BOOST_WARN_MESSAGE Scenario


Please remove redundant Scenario at the end.

paoloambrosio · 2017-09-05T05:35:33Z

features/specific/predicate_message.feature

+      }
+      """
+    When Cucumber runs the feature
+    Then the failure message "boost message" is output


Shall we condense all those scenarios into one scenario outline?

Scenario Outline: <check> Given the following feature: """ Feature: Feature name Scenario: Scenario name Given a step """ And a step definition file with: """ #include <boost/test/unit_test.hpp> #include <cucumber-cpp/autodetect.hpp> GIVEN("a step") { <check>(false, "boost message"); } """ When Cucumber runs the feature Then the scenario <passes or fails?> Examples: | check | passes or fails? | | BOOST_WARN_MESSAGE | passes | | BOOST_CHECK_MESSAGE | fails with message "boost message" | | BOOST_REQUIRE_MESSAGE | fails with message "boost message" |

Neat 👍 I was thinking about something like that - I just wasn't sure how to use it with C++ code.

konserw · 2017-09-13T21:39:54Z

I've created new feature file based on @paoloambrosio suggestions - now it contains more boost's macros.
Now I've also rebased against newest master.

Build is failing due to #169

paoloambrosio · 2017-09-14T21:56:03Z

After adding the other scenarios, there is again some duplication. I've tried experimenting with a single ScenarioOutline with all the examples, and I think it would be very readable:

    Examples:
      | check                                         | passes or fails?                    |
      | BOOST_CHECK(false)                            | fails                               |
      | BOOST_CHECK_MESSAGE(false, "boost message")   | fails with message "boost message"  |
      | BOOST_ERROR("boost message")                  | fails with message "boost message"  |
      | BOOST_FAIL("boost message")                   | fails with message "boost message"  |
      | BOOST_REQUIRE(false)                          | fails                               |
      | BOOST_REQUIRE_MESSAGE(false, "boost message") | fails with message "boost message"  |
      | BOOST_WARN(false)                             | passes                              |
      | BOOST_WARN_MESSAGE(false, "boost message")    | passes                              |

What do you think?

konserw · 2017-09-15T15:03:38Z

Looks good - I've commited this change.

paoloambrosio

Nice PR! It can be merged as soon as the build passes. Travis seems to be really slow, but I guess #169 will make it fail :-/

coveralls · 2017-10-08T16:41:31Z

Coverage increased (+0.003%) to 96.703% when pulling 59a3511 on konserw:issue81 into 0928f25 on cucumber:master.

…acros

coveralls · 2017-10-12T17:36:46Z

Coverage increased (+0.007%) to 96.707% when pulling d9a8f3c on konserw:issue81 into ffba658 on cucumber:master.

konserw · 2017-10-12T17:39:05Z

All green - I just had to rebase on top of master after change in travis scripts. I'll merge now.

konserw mentioned this pull request Aug 23, 2017

BOOST_PREDICATE_MESSAGE(boolean, string message) #81

Closed

konserw requested a review from muggenhor August 23, 2017 16:26

konserw added the bug label Aug 23, 2017

muggenhor requested changes Aug 23, 2017

View reviewed changes

muggenhor approved these changes Aug 23, 2017

View reviewed changes

muggenhor added this to the v0.5 milestone Aug 23, 2017

konserw force-pushed the issue81 branch from bea8cf2 to b89cc15 Compare August 24, 2017 17:11

muggenhor requested changes Aug 25, 2017

View reviewed changes

cucumber deleted a comment from coveralls Aug 25, 2017

muggenhor requested changes Aug 26, 2017

View reviewed changes

muggenhor approved these changes Sep 3, 2017

View reviewed changes

paoloambrosio reviewed Sep 5, 2017

View reviewed changes

cucumber deleted a comment from coveralls Sep 13, 2017

konserw force-pushed the issue81 branch from f7bac5c to 92544d2 Compare September 13, 2017 21:02

paoloambrosio approved these changes Sep 15, 2017

View reviewed changes

konserw mentioned this pull request Oct 8, 2017

BoostCalcQt segfaults on travis #169

Closed

konserw added 2 commits October 12, 2017 18:32

Fix issue cucumber#81 by intercepting messages from BOOST_*_MESSAGE m…

a2fa3f3

…acros

Feature file to test boost macros

d9a8f3c

konserw force-pushed the issue81 branch from 59a3511 to d9a8f3c Compare October 12, 2017 16:33

konserw added a commit to konserw/cucumber-cpp that referenced this pull request Oct 12, 2017

Merge cucumber#164 'Intercept messages from BOOST_*_MESSAGE macros'

ec26000

konserw merged commit d9a8f3c into cucumber:master Oct 12, 2017

konserw added a commit to konserw/cucumber-cpp that referenced this pull request Oct 12, 2017

Merge cucumber#164 'Intercept messages from BOOST_*_MESSAGE macros'

9fe2e73

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

		@@ -1,3 +1,7 @@
		Given /^a step definition file without support code:$/ do \|code\|

Fix issue #81 by intercepting messages from BOOST_*_MESSAGE macros #164

Fix issue #81 by intercepting messages from BOOST_*_MESSAGE macros #164

Conversation

konserw commented Aug 23, 2017 • edited by muggenhor Loading

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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 commented Aug 23, 2017

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

Choose a reason for hiding this comment

konserw commented Aug 25, 2017

konserw commented Aug 25, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paoloambrosio 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 Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

konserw commented Sep 13, 2017

paoloambrosio commented Sep 14, 2017

konserw commented Sep 15, 2017

paoloambrosio left a comment

Choose a reason for hiding this comment

coveralls commented Oct 8, 2017

coveralls commented Oct 12, 2017

konserw commented Oct 12, 2017

konserw commented Aug 23, 2017 •

edited by muggenhor

Loading

konserw Sep 13, 2017 •

edited

Loading