-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cucumber-cpp support for cgreen test framework. #129
Conversation
Cgreen repo lives here. |
Thanks for submitting this PR! :-) I've been quite busy to do an in-depth review but I'll do it ASAP. In the meantime some observations that will keep you busy:
|
Thanks for the update! I've addressed the following in this PR:
Maintaining the copyright information is a hard requirement for me though. Although the project reports "Copyright (c) 2010 Paolo Ambrosio" Unfortunately the role of PolySync Technologies, Inc. is not |
No other source file in the code base is polluted with copyright information, and we are not going to start now. I'm more than happy to accept PRs from "PolySync Technologies, Inc.", but we'll have to reach an agreement before this PR is considered for merging. The license chosen is among the most permissive in the OSS world. I'm surprised to receive this request from a company that has open-sourced software. Does it mean that the company would accept in its project a contribution assigning copyright to me? Contributors are recorded in the Changelog file when merging each PR, as well as being in the Git logs. The role of "PolySync Technologies, Inc." is contributing with that PR, and I have no problems attributing the change to the company instead of you. If this is agreeable, we don't need to discuss it anymore :-) If that is not enough, I think we should discuss this in the https://gitter.im/cucumber/contributors channel and hear the opinion of the others core contributors. |
Each repository under the cucumber GitHub organisation should have one and only one legal entity as copyright holder. Most of the repositories under the "Cucumber" GitHub organisation have an MIT license where the copyright holder is Cucumber Ltd. For historical reasons the copyright holder of this repo is Paolo Ambrosio. Cucumber Ltd (who owns the Cucumber GitHub organisation) will not allow more than one copyright holder for Cucumber Ltd repos, and I recommend the same policy for repositories like this one. If a project wants to stray from this recommendation they would have to fork and move outside the Cucumber organisation, something I'm sure @paoloambrosio isn't planning on ;-) So yes, these |
Ah that changelog file works perfectly! I'd missed that. My concerns will be wholly addressed by attributing changes to |
Copyright info removed from PR. |
Thanks for your understanding @Snewt - and glad you're happy with an honorable mention in the changelog! And thanks for your contribution. Cheers, |
I'm slowly going through this PR (apologies for that). It took me some time to realise that it doesn't work with the released version of Cgreen, that doesn't even come with CMake config files. I know, I missed that your contribution requires "Cgreen 1.1.0 or later"... but in my defence that version does not exist! Also Cgreen's CMake config in the master branch doesn't export the library's full path, making my local build fail since I keep Cucumber-CPP's dependencies in a separate directory (solved exporting the I also probably found the reason why you didn't add cgreen to the AppVeyor build for Windows: cgreen does not compile with Visual Studio, and indeed they don't build on Windows. Again, spent time solving a few issues but I gave up after a while (I don't even like Windows!). I like that you added the FizzBuzz example to test a pure C application. I would also create a More to follow after I take a look at the actual code ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to give it a better read to understand how it works.
I've left a few comments in this review.
On top of those, code conventions are inconsistent:
- underscores => camel case
if(...)\n{
=>if (...) {
(except in CMake files)- same for for statements
CMakeLists.txt
Outdated
# cgreen | ||
# | ||
if(NOT CUKE_DISABLE_CGREEN) | ||
find_package(cgreen QUIET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUIET
is not supported by cgreen-config.cmake
, and even if it was all other find_package
in the project do not use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUIET
is a cmake directive, it allows the cmake step to succeed if the find_package
function fails to find cgreen. Without the QUIET directive cmake can fail there. I'm happy to remove it if you want cmake to fail if the package is not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUIET
suppresses both success and failure messages. For consistency with the other find_package
calls, until , I suggest adding a more standard Findcgreen.cmake
.
Feel free to pull this commit on my personal repository that adds it on top of your PR.
#include <cgreen/mocks.h> // mocking functionality | ||
|
||
#include <cucumber-cpp/autodetect.hpp> | ||
#include <cucumber-cpp/internal/CukeEngine.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be included
#ifndef CUKE_CGREENDRIVER_HPP_ | ||
#define CUKE_CGREENDRIVER_HPP_ | ||
|
||
#include <cgreen/cgreen.h> // general unit testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the comment needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, will remove
@@ -0,0 +1,63 @@ | |||
#include <cgreen/cgreen.h> // general unit testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before
DriverTest::runAllTests(); | ||
} | ||
private: | ||
void stepInvocationInitsBoostTest() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not boost :-)
src/drivers/CgreenDriver.cpp
Outdated
__FILE__, | ||
__LINE__, | ||
"currentTest", | ||
arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling to follow what this is doing, but I don't think __FILE__
and __LINE__
can work here. Given that it's not in a macro, I believe they would be CgreenDriver.cpp
and 145
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm unclear what the concern here is. __FILE__
and __LINE__
are standard predefined macros that report source file name and line number, respectively. They are used here to give the cgreen reporter the info it requires for the proper output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't understand what this is doing:
va_list arguments;
memset(&arguments, 0, sizeof(va_list));
cgreenInterceptor.cgreenReporter->show_fail(
cgreenInterceptor.cgreenReporter,
__FILE__,
__LINE__,
"currentTest",
arguments);
Even if I remove the entire block, CgreenDriverTest
passes and the FizzBuzz example works (even failures are reported correctly if I introduce errors in the code).
@paoloambrosio good catch on that |
Recently resolved conflicts in the toml and added some Calc / C++ examples. Any more concerns to address? |
Does cgreen support building with cmake? couldn't we use externalproject for it? |
@konserw Cgreen does build with CMake. Your suggestion is a possibility but I followed the Boost / Qt lead, require a Cgreen install and use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Snewt can you rebase to master instead of merging from master? As you do it, I'd squash all commits like "whitespace commit", "removing travis reference to stale branch used during testing", "added a missing newline at the end of .travis.yml file", as well as the merges from Master.
.travis.yml
Outdated
@@ -22,13 +22,18 @@ matrix: | |||
compiler: gcc | |||
env: GMOCK_VER=1.8.0 VALGRIND_TESTS=ON | |||
|
|||
before_script: | |||
- git clone https://github.com/cgreen-devs/cgreen.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that after several months there is still no sign of a 1.1.0 release, I think we should fix our build to a specific git hash. I want to avoid our build failing when someone makes incompatible changes to Cgreen.
What I had in mind is approach simmilar to what we have for gtest/gmock, especially since there is no cgreen package available for ubuntu (which we use in CI), i.e. if cgreen is available in system use that, otherwise use externalproject to clone and build cgreen for cucumber. |
I'm not a fan of externalproject so I'm happy to merge this PR with the current CMake file. |
CMakeLists.txt
Outdated
|
||
# | ||
# Valgrind | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build errors are due to this section being duplicated (suspect during rebasing conflict).
For your next PR, I suggest rebasing earlier instead of merging from master. Apologies for not spotting it earlier: it could have saved you a headache. You should be able to solve that problem with the following instructions. Start an intereactive rebase from master:
Unstage changes: Edit Commit the changes with
Continue rebasing: |
@paoloambrosio thanks for the pointers, rebase rather that merge is a workflow I need to master. Also, thanks for putting the time into spotting the duplicate code / bad rebase conflict resolution. |
@paoloambrosio was experimenting with a fresh VM tonight and realized that the cgreen package is no longer found on Linux, the OS X builds on Travis run the cgreen examples but the linux builds do not. I think I've spotted the culprit (cgreen_FOUND vs CGREEN_FOUND), will try and fix in the next couple days |
Fixed the |
update, I have a PR up on the cgreen repository to fix the leak that is causing my the cgreen valgrind tests to fail. |
Okay, the Cgreen team has merged my PR. Had to add a few lines to the CgreenDriver source after bumping the version to account for some new error message output but it looks like we actually have a known good state in the hash we checkout in the travis file. |
hmm, a couple AppVeyor builds are failing with a FindBoost.cmake error, not sure how to tackle that one. @paoloambrosio I'm definitely open to suggestions. |
It looks like the variables in the appveyor.yml passed as arguments to cmake don't like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to look at most of your code changes. I'm guessing you're more experienced at C than C++, so have tried to write my comments in such a way that they don't depend too much on assumed C++ knowledge. Overall it looks like you're on the right track. Please don't be disheartened by the amount of comments I have. Most of them are about details that should be relatively easy to address individually, although I do recognize there's a large number of them.
.travis.yml
Outdated
@@ -22,13 +22,19 @@ matrix: | |||
compiler: gcc | |||
env: GMOCK_VER=1.8.0 VALGRIND_TESTS=ON | |||
|
|||
before_script: | |||
- git clone https://github.com/cgreen-devs/cgreen.git | |||
- pushd cgreen && git checkout 7030597 && popd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also just use git -C cgreen checkout $committish
.
.travis.yml
Outdated
before_script: | ||
- git clone https://github.com/cgreen-devs/cgreen.git | ||
- pushd cgreen && git checkout 7030597 && popd | ||
- mkdir cgreen/build && pushd cgreen/build && cmake .. && make && sudo make install && popd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer for this to be similar to the way we build cucumber-cpp itself, e.g.:
cmake -E make_directory cgreen/build
cmake -E chdir cgreen/build cmake -G Ninja ..
cmake --build cgreen/build
cmake --build cgreen/build --target install
Additionally, the root access (sudo
) shouldn't be necessary. If instead you alter the cmake configuration command to be:
cmake -E chdir cgreen/build cmake -G Ninja -DCMAKE_INSTALL_PREFIX=${HOME}/usr ..
Then you can pass -DCMAKE_PREFIX_PATH=${HOME}/usr
to the cmake invocation for cucumber-cpp itself and it should just find it.
CMakeLists.txt
Outdated
# cgreen | ||
# | ||
|
||
if(NOT CUKE_DISABLE_CGREEN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an option near the top of this file like there exists for other optionally disabled features.
README.md
Outdated
@@ -27,6 +27,8 @@ It relies on a few libraries: | |||
Optional library for Boost Test driver: *test*. | |||
* [GTest](http://code.google.com/p/googletest/) 1.6 or later. | |||
Optional for the GTest driver. By default downloaded and built by CMake. | |||
* [Cgreen](https://github.com/cgreen-devs/cgreen) 1.1.0 or later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this comment should be marked in some way to indicate that it's instead expected to work with what will eventually become cgreen 1.1.0
cmake/modules/Findcgreen.cmake
Outdated
if(CGREEN_FOUND) | ||
set(CGREEN_LIBRARIES ${CGREEN_LIBRARY}) | ||
set(CGREEN_INCLUDE_DIRS "${CGREEN_INCLUDE_DIR}") | ||
set(CGREEN_EXECUTABLE "${CGREEN_RUNNER}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it if instead you would add import targets here, similar to what we do in FindGMock.cmake
. That should make it significantly easier to actually use this.
For example this (untested, but it should be in the right direction):
add_library(Cgreen::Cgreen UNKNOWN IMPORTED)
set_target_properties(Cgreen::Cgreen PROPERTIES
IMPORTED_LINK_INTERFACE_LANGUAGES "C"
IMPORTED_LOCATION "${CGREEN_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${CGREEN_INCLUDE_DIR}"
)
add_executable(Cgreen::runner IMPORTED) # This may need the GLOBAL keyword as well, needs to be tested to confirm either way
set_target_properties(Cgreen::runner PROPERTIES
IMPORTED_LOCATION "${CGREEN_RUNNER}"
)
Then just use those two targets where currently you use these intermediate variables instead. (Uses of the ${CGREEN_INCLUDE_DIRS}
variable can be removed, as they're now covered by the usage requirements in the Cgreen::Cgreen
target properties).
src/drivers/CgreenDriver.cpp
Outdated
|
||
bool CgreenStep::initialized = false; | ||
|
||
boost::function< void() > currentTestBody; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you make this a static
class member instead?
} | ||
private: | ||
void stepInvocationInitsCgreenTest() { | ||
std::cout << "= Init =" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you really want .flush() to be called here I suggest you just terminate the string with "\n"
instead of streaming in std::endl
. std::endl
does nothing more than streaming in "\n"
followed by calling .flush()
.
FYI: some people think it takes care of converting to the current OS' end-of-line character sequence: it doesn't, that's what the underlying stream itself already does for "\n"
(exactly like not passing the "b" flag to fopen() does for C, in fact: on GNU's libstdc++ it uses the C library).
travis.sh
Outdated
@@ -31,3 +32,7 @@ if [ -f $BOOST ]; then | |||
cucumber examples/Calc | |||
wait | |||
fi | |||
if [ -f $CGREEN ]; then | |||
$CGREEN >/dev/null & | |||
cucumber examples/FizzBuzz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a wait
call after cucumber
. That makes sure that to wait for $CGREEN
to terminate (and to use its exit code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/drivers/CgreenDriver.cpp
Outdated
va_list argPtr; | ||
|
||
va_start(argPtr, format); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this a lot: empty lines between related code. Could you try to remove some of that?
In this case, for example, I wouldn't expect to see a single empty line between the va_list
declaration and the va_end
call, as it's all related and logically doing one thing.
src/drivers/CgreenDriver.cpp
Outdated
|
||
static CukeCgreenInterceptor cgreenInterceptor; | ||
|
||
std::string CukeCgreenInterceptor::cgreenOutput = std::string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the copy-initialization. In C++ all objects of types that have a default constructor (which is almost all non-POD types) will get it called when no initializer is specified (so no need to worry about uninitialized memory here).
@muggenhor thanks for all the great feedback, the changes you requested are well justified. I really appreciate you taking the time to explain your reasoning. |
@muggenhor I've tried to address your requested changes. Thanks again for the comments, I learned some things implementing them. I'm unsure about my README.md wording still, I don't know the best way to indicate a supported cgreen version when there hasn't been an actual version in some time. I've been using cgreen and cucumber-cpp in tandem for some time now and feel like even at the stage of development cgreen is currently at it is still a valuable tool. It didn't seem like a commit hash in the README was appropriate though. |
Also, it appears the |
cmake/modules/Findcgreen.cmake
Outdated
set_target_properties(Cgreen::runner PROPERTIES | ||
IMPORTED_LOCATION "${CGREEN_RUNNER}" | ||
) | ||
include_directories(${CGREEN_INCLUDE_DIR}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please delete this line: we don't want to unconditionally add this to the include path of everything.
examples/FizzBuzz/CMakeLists.txt
Outdated
add_executable(FizzBuzzSteps features/step_definitions/FizzBuzzSteps) | ||
target_link_libraries(FizzBuzzSteps FizzBuzz ${CUKE_LIBRARIES} Cgreen::Cgreen) | ||
else() | ||
message("cgreen package required for FizzBuzz example") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just don't: we don't do this for any of the other test-framework-specific tests and executables either, so lets not start doing this here. It just increases the log verbosity with messages most people will tend to ignore.
@@ -0,0 +1,11 @@ | |||
#ifdef __cplusplus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an include guard to this header, e.g.:
#ifndef CUKE_FIZZBUZZREPORTER_H_
#define CUKE_FIZZBUZZREPORTER_H_
// contents
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern "C" { | ||
#endif | ||
|
||
#include <string.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using anything from this header: don't include it here.
|
||
#include <string.h> | ||
|
||
void fizzBuzzReporter(unsigned int input, char * reportBuffer, size_t bufferSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t
depends on #include <stddef.h>
, please do
src/drivers/CgreenDriver.cpp
Outdated
std::string CukeCgreenInterceptor::cgreenOutput; | ||
TestSuite * CukeCgreenInterceptor::cgreenSuite = NULL; | ||
TestReporter * CukeCgreenInterceptor::cgreenReporter = NULL; | ||
boost::function< void() > CukeCgreenInterceptor::currentTestBody = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a pointer, just leave it default initialized (i.e. remove the = NULL
bit).
src/drivers/CgreenDriver.cpp
Outdated
cgreenOutput += buffer; | ||
cgreenOutput += "\n\n"; | ||
} | ||
va_end(argPtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the va_end
to directly after vsnprintf
.
@muggenhor regarding the request to remove the blanketed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add the target_include_directories
calls. You just seem to have forgotten to ensure that the cucumber-cpp
target itself gets linked to Cgreen::Cgreen
. One of my review comments (in src/CMakeLists.txt
) explains how to fix that.
.travis.yml
Outdated
- cmake -E chdir cgreen/build cmake -G Ninja -DCMAKE_INSTALL_PREFIX:PATH=${HOME}/usr .. | ||
- cmake --build cgreen/build | ||
- cmake --build cgreen/build --target install | ||
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then sudo ldconfig; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh btw, this (ldconfig
) can go: we're not installing in a system-wide directory anymore.
src/CMakeLists.txt
Outdated
@@ -24,6 +24,10 @@ if(Boost_UNIT_TEST_FRAMEWORK_FOUND) | |||
list(APPEND CUKE_SOURCES drivers/BoostDriver.cpp) | |||
endif() | |||
|
|||
if(CGREEN_FOUND) | |||
list(APPEND CUKE_SOURCES drivers/CgreenDriver.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like is done for GTest (few lines above): you need to ensure Cgreen gets linked to, like this:
list(APPEND CUKE_DEP_LIBRARIES Cgreen::Cgreen)
@muggenhor ah thanks, I'd misremembered. |
src/drivers/CgreenDriver.cpp
Outdated
|
||
class CukeCgreenInterceptor { | ||
public: | ||
const std::string getCgreenOutput(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two options:
- Remove the
const
that will allow C++11 (and beyond) compilers to perform a (cheaper) move instead of copy - Return a const reference instead, thus postponing the copy until it's really necessary
src/drivers/CgreenDriver.cpp
Outdated
cgreenInterceptor.reset_reporter(); | ||
} | ||
|
||
cgreenInterceptor.currentTestBody = boost::bind(&CgreenStep::body, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the type of currentTestBody
to BasicStep*
. Then you can just call currentTestBody->body()
inside of currentTest
. That's a lot more light-weight than using boost::bind
and boost::function
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body()
function of the BasicStep class is protected.
} | ||
private: | ||
void stepInvocationInitsCgreenTest() { | ||
std::cout << "= Init =\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line
src/drivers/CgreenDriver.cpp
Outdated
|
||
va_list argPtr; | ||
va_start(argPtr, format); | ||
vsnprintf(buffer, sizeof(buffer), format, argPtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capture the return value and verify that it's not negative before using buffer
src/drivers/CgreenDriver.cpp
Outdated
} | ||
|
||
CukeCgreenInterceptor::~CukeCgreenInterceptor() { | ||
destroy_test_suite(cgreenSuite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it: this will always be called by cucumber (because you've made it a global variable), but you're not always initializing these to be non-NULL. So please ensure that you're not destroying NULL test suites and reporters.
"/" | ||
}; | ||
|
||
class CukeCgreenInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class smells: it's basically a singleton (the single static
instance of this class) with every member being a singleton too (static
member variables)! I suspect the quality of your destructor (which to me looks like it will trigger NULL pointer dereferences) is the result of this.
Please:
- do without any singleton at all, or;
- don't nest singletons and make the top-level one closer to the singleton design pattern with lazy initialization (use a smart pointer for the instance pointer to ensure the destructor actually gets called).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised to conform to my understanding of what a lazy initialized singleton looks like. The project doesn't seem to enforce c++11 so I can't assume std::unique_ptr is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, you should assume only C++98, as we currently still are able to build with just that. You don't need std::unique_ptr
for a singleton though, that just makes it slightly easier to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@muggenhor got it. Well I took a stab at implementing the lazy evaluate singleton you requested without the unique_ptr. I'm not dynamically allocating memory for the singleton itself though there are fields in it that are allocated and freed in its destructor. My assumption is that that memory is being successfully managed since the Valgrind tests pass. Let me know what your thoughts are.
I've been unable to reproduce the BoostCalculatorQtSteps failure that's breaking the Travis/Linux builds locally but am investigating. I triggered a build of the current state of master from my fork and it seems to be broken there too. |
@Snewt - FYI if you rebase now onto master travis build should pass (issue has been worked-around in master) |
Don't really like merges from master. Rebase is your friend, like @konserw suggested. |
As a user I want to use cgreen as an alternative test framework to gtest and boost in order to test native C complete with mocking capability. cucumber-cpp now recognizes and harnesses cgreen as a testing framework. The cgreen reporting is intercepted and presented on the failure of a cucumber test. An example (FizzBuzz) is written as an example of a testable C progam and is included in the examples directory. Unit tests for the cgreen driver are modeled after the boost and gtest drivers and are passing.
In order to show how the Cgreen framework can be used with C++, Cgreen tests, modeled after the Boost tests were written for the Calc and CalcQt examples.
Prior to this commit the version of Cgreen checked out in the `.travis.yml` file introduced a memory leak to the CgreenDriver. After this commit the version of Cgreen checked out by Travis reflects a version of Cgreen with that memory leak fixed. Due to the bump in Cgreen version that introduced new cgreen error output, some "black box" functionality of the cucumber-cpp CgreeDriver was being reported. This commit also truncates that information so it isn't visible in a failed tests' error output.
travis.sh
Outdated
; do | ||
if [ -f "${TEST}" -a -n "${DISPLAY:-}" ]; then | ||
"${TEST}" 2> /dev/null & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change this line - it will produce a lot of garbage if you change it
@@ -92,4 +107,7 @@ if [ -f "${TEST}" ]; then | |||
wait % | |||
fi | |||
|
|||
killXvfb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also don't change this one (and following)
@konserw thanks for pointing those issues out. Looks like my build is still failing though. Any more suggestions? I experimented with another branch and the failure seems to be rooted in this block of the
|
Prior to this commit this feature's implementation did not fully conform to C++ standards and convention. This lead to inconsistencies and ambiguity in the source. After this commit the implementation should conform somewhat more to this project's standards as well as a C++ convention. Other changes included in this commit: - cmake revisions to match the functionality and usage for this project's gmock dependency. - travis file cleanup, the logic applied to Cgreen steps did not fully conform to the project's other dependencies. - Documentation regarding supported Cgreen versions (or more accurately lack of versions)
Hallo, Despite really appreciating the effort the @shnewto has put into this PR, I also think it is not a good direction to add support for various (unit) test frameworks to cucumber-cpp. This would make cucumber-cpp dependent on each and every test framework it is embedded in, while it should be the other way around: the test of the user of cucumber-cpp should be depending on both test framework it runs on and cucumber-cpp, the dependency for cucumber-cpp towards the test framework should be injected by the user of the test by means of a - by cucumber-cpp - well defined interface (kind of as describe in issue #154 ). This PR will be closed without being merged into main (besides, this PR seems to be really out of date). However, contributors to this PR are more than welcome to participate in the design of a plugin / dependency injection system for test drivers in cucumber-cpp. Kind regards, |
As a user I want to use cgreen as an alternative test framework to gtest
and boost in order to test native C complete with mocking capability.
cucumber-cpp now recognizes and harnesses cgreen as a testing framework.
The cgreen reporting is intercepted and presented on the failure of a
cucumber
test.
An example (FizzBuzz) is written as an example of a testable C progam
and
is included in the examples directory.
Unit tests for the cgreen driver are modeled after the boost and gtest
drivers and are passing.