add cpp tag expressions#241
Conversation
|
@daantimmer your review would be welcome too. |
There was a problem hiding this comment.
At a glance it looks like there are no tests against the testdata folder. To ensure all parsers work the same it would be preferable if there were.
For example see:
https://github.com/cucumber/tag-expressions/tree/main/python/tests/data
I think it would also allow some of hand written tests to be deleted.
Thanks for the comments. Tests against the https://github.com/hs515/tag-expressions/tree/cpp-tag-expressions/cpp/tests. All listed tests passed. (Ln25 and Ln29 in testdata/parsing.yml are identical, so only test one.) Running main() from ./googletest/src/gtest_main.cc [----------] 7 tests from EvaluationsTest [----------] 22 tests from ParsingTest [----------] Global test environment tear-down |
|
Interesting. Will have a look. The reason why is that I've written exactly the same thing, but based on the JavaScript+python implementation. Not only that, I've also written the whole runner part too, which I am now covering to a full messages based implementation. Just so you know, I also have the cucumber expressions part ready ;-). But if this is feature compete just as mine then great. Than I don't have to do the work to move my implementation here for the tag expression part. I'll happily have a review on the implementation. |
There was a problem hiding this comment.
What is your reasoning on keeping some definitions in the header and some in the source? IMHO all but templates definitions should be in a source file.
| @@ -0,0 +1,176 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
| namespace cucumber { | ||
| namespace tag_expressions { |
There was a problem hiding this comment.
Since C++17 (that you are requiring) we have nested namespaces, see (8): https://en.cppreference.com/w/cpp/language/namespace.html
It'll make everything a tad more readable by having less indentations.
| namespace cucumber { | |
| namespace tag_expressions { | |
| namespace cucumber::tag_expressions { |
Don't forget to remove the extra curly brace at the end.
| * @return true if expression evaluates to true with values | ||
| * @return false otherwise | ||
| */ | ||
| virtual bool evaluate(const std::set<std::string>& values) const = 0; |
There was a problem hiding this comment.
Since you are not using SonarQube: SonarQube will give a warning on not adding an explicit compare functor. In general it suggests the following for all std::map and std::set:
| virtual bool evaluate(const std::set<std::string>& values) const = 0; | |
| virtual bool evaluate(const std::set<std::string, std::less<>>& values) const = 0; |
Please follow SonarQube's advice
There was a problem hiding this comment.
Using shared_ptr's is generally a design flaw. The same applies here. All of these shared_ptr's should in fact be unique_ptr's.
You'll never have shared ownership over a single instance of an Expression instance. Only a unique ownership.
|
|
||
| std::string to_string() const override; | ||
|
|
||
| const std::string& name() const { return name_; } |
There was a problem hiding this comment.
| const std::string& name() const { return name_; } | |
| std::string_view name() const { return name_; } |
| explicit And(std::vector<std::shared_ptr<Expression>> terms) | ||
| : terms_(std::move(terms)) {} | ||
|
|
||
| bool evaluate(const std::set<std::string>& values) const override { | ||
| for (const auto& term : terms_) { | ||
| if (!term->evaluate(values)) { | ||
| return false; // SHORTCUT: Any false makes the expression false | ||
| } | ||
| } | ||
| return true; // OTHERWISE: All terms are true | ||
| } | ||
|
|
||
| std::string to_string() const override; | ||
|
|
||
| const std::vector<std::shared_ptr<Expression>>& terms() const { return terms_; } | ||
|
|
||
| private: | ||
| std::vector<std::shared_ptr<Expression>> terms_; |
There was a problem hiding this comment.
I know you took this implementation from Python. But it makes no sense that one can have more than two or less than two arguments to And and Or. I propose to change the constructors to take two arguments left and right:
| explicit And(std::vector<std::shared_ptr<Expression>> terms) | |
| : terms_(std::move(terms)) {} | |
| bool evaluate(const std::set<std::string>& values) const override { | |
| for (const auto& term : terms_) { | |
| if (!term->evaluate(values)) { | |
| return false; // SHORTCUT: Any false makes the expression false | |
| } | |
| } | |
| return true; // OTHERWISE: All terms are true | |
| } | |
| std::string to_string() const override; | |
| const std::vector<std::shared_ptr<Expression>>& terms() const { return terms_; } | |
| private: | |
| std::vector<std::shared_ptr<Expression>> terms_; | |
| And(std::unique_ptr<Expression> left, std::unique_ptr<Expression> right) | |
| : left{ std::move(left) } | |
| , right{ std::move(right) } | |
| bool evaluate(const std::set<std::string>& values) const override { | |
| left->Evaluate(values) && right->Evaluate(values); | |
| } | |
| std::string to_string() const override; | |
| std::pair<const Expression*, const Expression*> terms() const { return {left.get(), right.get()}; } | |
| private: | |
| std::unique_ptr<Expression> left; | |
| std::unique_ptr<Expression> right; |
| bool evaluate(const std::set<std::string>& values) const override { | ||
| (void)values; // Unused parameter |
There was a problem hiding this comment.
| bool evaluate(const std::set<std::string>& values) const override { | |
| (void)values; // Unused parameter | |
| bool evaluate([[maybe_unused]] const std::set<std::string>& values) const override { |
| explicit TagExpressionError(const std::string& message) | ||
| : std::runtime_error(message) {} |
There was a problem hiding this comment.
| explicit TagExpressionError(const std::string& message) | |
| : std::runtime_error(message) {} | |
| using std::runtime_error::runtime_error; |
|
|
||
| } // namespace tag_expressions | ||
| } // namespace cucumber | ||
|
No newline at end of file |
| bool TokenInfo::is_binary() const { | ||
| return keyword == "or" || keyword == "and"; | ||
| } |
There was a problem hiding this comment.
This function is unused
| bool TokenInfo::is_binary() const { | |
| return keyword == "or" || keyword == "and"; | |
| } |
| : keyword(std::move(kw)), precedence(prec), assoc(a), token_type(tt) {} | ||
|
|
||
| bool is_operation() const { return token_type == TokenType::OPERATOR; } | ||
| bool is_binary() const; |
There was a problem hiding this comment.
This function is unused
| bool is_binary() const; |
There was a problem hiding this comment.
You are missing the Precedence tests like( this is coming from my implementation ):
TEST(TestToken, TestPrecedence)
{
EXPECT_THAT(OR.HasLowerPrecedenceThan(OR), testing::IsTrue());
EXPECT_THAT(OR.HasLowerPrecedenceThan(AND), testing::IsTrue());
EXPECT_THAT(OR.HasLowerPrecedenceThan(NOT), testing::IsTrue());
EXPECT_THAT(AND.HasLowerPrecedenceThan(AND), testing::IsTrue());
EXPECT_THAT(AND.HasLowerPrecedenceThan(OR), testing::IsFalse());
EXPECT_THAT(AND.HasLowerPrecedenceThan(NOT), testing::IsTrue());
EXPECT_THAT(NOT.HasLowerPrecedenceThan(NOT), testing::IsFalse());
EXPECT_THAT(NOT.HasLowerPrecedenceThan(OR), testing::IsFalse());
EXPECT_THAT(NOT.HasLowerPrecedenceThan(AND), testing::IsFalse());
}
TEST(TestToken, TestPrecedenceWithParenthesis)
{
EXPECT_THAT(OR.HasLowerPrecedenceThan(OPEN_PARENTHESIS), testing::IsFalse());
EXPECT_THAT(OR.HasLowerPrecedenceThan(CLOSE_PARENTHESIS), testing::IsFalse());
EXPECT_THAT(AND.HasLowerPrecedenceThan(OPEN_PARENTHESIS), testing::IsFalse());
EXPECT_THAT(AND.HasLowerPrecedenceThan(CLOSE_PARENTHESIS), testing::IsFalse());
EXPECT_THAT(NOT.HasLowerPrecedenceThan(OPEN_PARENTHESIS), testing::IsFalse());
EXPECT_THAT(NOT.HasLowerPrecedenceThan(CLOSE_PARENTHESIS), testing::IsFalse());
}
| } | ||
| }; | ||
|
|
||
| std::vector<Token> operations; |
There was a problem hiding this comment.
| std::vector<Token> operations; | |
| std::stack<Token> operations; |
| throw TagExpressionError("Invalid expression: Expected exactly one result"); | ||
| } | ||
|
|
||
| return expressions.back(); |
There was a problem hiding this comment.
Not entirely sure about this one:
| return expressions.back(); | |
| return std::move(expressions.back()); |
| if(MSVC) | ||
| add_compile_options(/W4 /WX) | ||
| else() | ||
| add_compile_options(-Wall -Wextra -Wpedantic -Werror) | ||
| endif() |
There was a problem hiding this comment.
Only do this when building in standalone mode.
Leave additional warnings (and especially -Werror) off when being consumed as a project.
| target_link_libraries(example PRIVATE cucumber::tag-expressions) | ||
|
|
||
| # Enable testing | ||
| enable_testing() |
There was a problem hiding this comment.
don't always enable_testing put that under a cmake options flag.
| find_package(GTest QUIET) | ||
| if(GTest_FOUND) | ||
| add_executable(tag_expressions_test | ||
| tests/test_errors.cpp | ||
| tests/test_evaluations.cpp | ||
| tests/test_parsing.cpp | ||
| ) | ||
| target_link_libraries(tag_expressions_test | ||
| PRIVATE | ||
| cucumber::tag-expressions | ||
| GTest::gtest | ||
| GTest::gtest_main | ||
| ) | ||
|
|
||
| add_test(NAME tag_expressions_test COMMAND tag_expressions_test) | ||
| endif() |
There was a problem hiding this comment.
Put all of these under that same cmake options flag to 'enable testing build'. For example this is what I do for my libraries:
option(CCR_BUILD_TESTS "Enable build of the tests" ${CCR_DEFAULTOPT})
option(CCR_ENABLE_COVERAGE "Enable compiler flags for code coverage measurements" Off)
|
In general what I am missing are some good practices:
For the latter point, if you want, we (not cucumber, but Philips) have a container available for CI that allows building for all of the above targets (except for native windows, just use a windows github runner for that) |
|
@mpkorstanje besides my comments the functionality itself is functionally the same as what I did. There are some taste-differences in how to write C++, but give 10 engineers the same assignment and you get 10 different implementations :-). I do think my comments are valid and require downstream changes. What might be worth the effort is to add a yaml parser dependency for a test build (it's what I did). Which allowed me to straight up use the official testdata. Which means no unit tests need to be modified when the testdata is expanded/updated. See: std::vectortesting::TestInfo* RegisterMyTests() as an example of how I used |
|
Also, @hs515 be sure to have a look here: https://github.com/philips-software/amp-cucumber-cpp-runner/tree/feature/rewrite-to-use-cucumber-messages as you don't have to recreate everything that I already have ;-) |
| * | ||
| * @param name Tag name to represent as a literal | ||
| */ | ||
| explicit Literal(std::string name) : name_(std::move(name)) {} |
There was a problem hiding this comment.
Going to comment once for all initializers. Please use uniform initialization to initialize variables and members: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax
| explicit Literal(std::string name) : name_(std::move(name)) {} | |
| explicit Literal(std::string name) : name_{std::move(name)} {} |
daantimmer
left a comment
daantimmer
left a comment
There was a problem hiding this comment.
I like your effort. Its functionally the same as what I already did. Minus the comments I've given.
However, there are, for me, some blocking comments that need to be resolved before this could be merged. Feel free to discuss them! Here on on the cucumber discord (there is no C++ channel yet)
|
@hs515 I do have a few more questions about your motivation for contributing. And I'm sorry I didn't lead with these questions, but Daans comments prompted to think about this.
These question are a bit of a departure from the past practices, but with LLM assistance writing plausible looking code has become easy enough that the effort involved in writing that code is no longer a signal that the author had a genuine and sustained interest in contributing. @daantimmer good points all. Cheers! |
@daantimmer, thank you very much for your decent review. I will address @mpkorstanje's questions and then come back to your comments. Many thanks. |
I have noticed the #313 merge request. I am not sure how you are going to react to that. But no matter to merge the whole thing or to break it down and merge it in parts, I think it makes sense to have a C++ tag-expressions implementation in the repo. If the community decides to merge the C++ tag-expressions from somewhere else, I can close my current merge request.
Let me know whether I should continue addressing @daantimmer 's comments? Thanks. |
|
Thanks for the clarification. Much appreciated! With that in mind we'd be happy to host the cpp implementation of tag expressions.
Right now that will be for Urs Fässler to decide. We mostly seem to have a dearth people interested in the original cucumber-cpp so I can't imagine any tears will be shed if does get archived/merged/replaced.
I've never found the writing part to be the bottleneck. Reviewing on the other hand is more tedious. The project mostly runs on volunteers and I do not want to waste their time asking them to review unreviewed code. So I would ask that you've made sure to fully understand each line before it is contributed. Though with 20+ years of experience I'm sure you'll be able to pick out the generated mistakes quite quickly so that is less of a concern than it originally was. |
Great! Then I will start addressing Daan's comments.
Running tests is not difficult today. GTest and some other test frameworks are very easy to use. However, Cucumber-cpp does still have its own advantages, such as human-readable feature files, tagging requirements to scenarios, adding tests without recompilation, and generating reports. IMO, if an easy-to-use cucumber-cpp was in place, it would gain more interest. |
Chicken, meet egg. 😃 |
I will look at these open questions.
@daantimmer , do you mind sharing how to use your (Philips) container that allows building for the above targets? |
@daantimmer , amazing, you have done a great work! Why didn't you break your work down and create pull requests to the cucumber repo? I think that could be more visible. |
Thanks for your great comments. I am addressing as many as I can.
For a standalone project, I would like to agree to add a yaml parser dependency. But for a project that is itself a dependency, my preference would be opposite (not to add unnecessary dependencies). |
Multiple reasons:
My personal goal would be to get everything towards cucumber. Most of the separate functionalities are already decoupled for the most part, bar some "timestamp" and "duration" utility functions. That branch that I shared is nearing completion. Some minor cleanups to the current formatters are required. And some documentation on how to use everything. Only "major" things I am lacking so far:
I'll need to write additional unit tests, add everything is currently only tested though the compatibility kit layer. Which gives me 85% coverage, but I want to be higher. Also, unit tests :-) (I had them for my first implementation, but when I rewrote based on cucumber-js it was easier to get the implementation in first) Everything else is quite complete compared to cucumber-js. (Which I used as an architectural/design example). |
|
I copied the tag-expressions/cpp into another publish repo, and ran SonarQube analysis and coverage tests. Here is the result: https://sonarcloud.io/project/overview?id=hs515_cpp-tag-expressions. I have ensured it runs in CI with GCC. Next step, I will continue trying
@mpkorstanje, should I or @daantimmer close review comments? |
|
I have another cucumber-cpp runner project that depends on the existing cucumber-cpp and cucumber-gherkin projects. The existing cucumber-cpp supports snippets generation and REGEX_PARAM argument type lookups, but the current cucumber-cpp does not support cucumber expressions. My running project can also generate HTML outputs. You can take a look at https://github.com/hs515/cuke-cpp.
|
|
@hs515 I'll take a look and steal it when possible. First things first though which is to wrap up the current PR. Don't want to feature creep it :-) |
|
Auto build and run CI passed for GCC, Clang, and MSVC. Build activities can be found at https://github.com/hs515/cpp-tag-expressions/actions/runs/21267166216. |
3 participants
🤔 What's changed?
Add a CPP implementation of the Tag Expressions parser.
⚡️ What's your motivation?
This will allow CPP-based Cucumber runner to have a native tag expression parser.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.