feat!: rewrite to use cucumber messages #273
Conversation
… compatibility kit
…r to catch errors
…ta, doc-strings, empty, examples-tables and minimal
…ooks-beforeall-error, hooks, hooks-attachment and hooks-conditional compatibility tests
✅
|
| Descriptor | Linter | Files | Fixed | Errors | Warnings | Elapsed time |
|---|---|---|---|---|---|---|
| ✅ ACTION | actionlint | 6 | 0 | 0 | 0.39s | |
| ✅ CPP | clang-format | 181 | 0 | 0 | 0 | 1.3s |
| ✅ DOCKERFILE | hadolint | 1 | 0 | 0 | 0.42s | |
| ✅ JSON | jsonlint | 8 | 0 | 0 | 0.16s | |
| ✅ JSON | prettier | 8 | 6 | 0 | 0 | 0.5s |
| markdownlint | 6 | 3 | 13 | 0 | 0.8s | |
| ✅ MARKDOWN | markdown-table-formatter | 6 | 3 | 0 | 0 | 0.28s |
| ✅ REPOSITORY | git_diff | yes | no | no | 0.03s | |
| ✅ REPOSITORY | grype | yes | no | no | 28.17s | |
| ✅ REPOSITORY | ls-lint | yes | no | no | 0.06s | |
| ✅ REPOSITORY | secretlint | yes | no | no | 2.12s | |
| ✅ REPOSITORY | syft | yes | no | no | 1.25s | |
| ✅ REPOSITORY | trivy | yes | no | no | 5.36s | |
| ✅ REPOSITORY | trivy-sbom | yes | no | no | 0.12s | |
| ✅ REPOSITORY | trufflehog | yes | no | no | 2.2s | |
| lychee | 73 | 1 | 0 | 4.3s | ||
| ✅ YAML | prettier | 10 | 0 | 0 | 0 | 0.52s |
| ✅ YAML | v8r | 10 | 0 | 0 | 6.25s | |
| ✅ YAML | yamllint | 10 | 0 | 0 | 0.46s |
Detailed Issues
⚠️ SPELL / lychee - 1 error
[404] https://github.com/yourname/amp-cucumber-cpp-runner.git | Network error: Not Found
📝 Summary
---------------------
🔍 Total..........153
✅ Successful.....152
⏳ Timeouts.........0
🔀 Redirected.......0
👻 Excluded.........0
❓ Unknown..........0
🚫 Errors...........1
Errors in CONTRIBUTING.md
[404] https://github.com/yourname/amp-cucumber-cpp-runner.git | Network error: Not Found
⚠️ MARKDOWN / markdownlint - 13 errors
CHANGELOG.md:26 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:38 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:47 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:53 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:61 MD024/no-duplicate-heading Multiple headings with the same content [Context: "⚠ BREAKING CHANGES"]
CHANGELOG.md:65 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:70 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Bug Fixes"]
CHANGELOG.md:79 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:90 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
CHANGELOG.md:98 MD024/no-duplicate-heading Multiple headings with the same content [Context: "⚠ BREAKING CHANGES"]
CHANGELOG.md:102 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Features"]
CHANGELOG.md:127 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Bug Fixes"]
CHANGELOG.md:134 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Chores"]
See detailed reports in MegaLinter artifacts
Your project could benefit from a custom flavor, which would allow you to run only the linters you need, and thus improve runtime performances. (Skip this info by defining FLAVOR_SUGGESTIONS: false)
- Documentation: Custom Flavors
- Command:
npx [email protected] --custom-flavor-setup --custom-flavor-linters ACTION_ACTIONLINT,CPP_CLANG_FORMAT,DOCKERFILE_HADOLINT,JSON_JSONLINT,JSON_PRETTIER,MARKDOWN_MARKDOWNLINT,MARKDOWN_MARKDOWN_TABLE_FORMATTER,REPOSITORY_GIT_DIFF,REPOSITORY_GRYPE,REPOSITORY_LS_LINT,REPOSITORY_SECRETLINT,REPOSITORY_SYFT,REPOSITORY_TRIVY,REPOSITORY_TRIVY_SBOM,REPOSITORY_TRUFFLEHOG,SPELL_LYCHEE,YAML_PRETTIER,YAML_YAMLLINT,YAML_V8R
Test Results26 tests 26 ✅ 11s ⏱️ Results for commit c82fa49. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 291 out of 372 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return {}; | ||
| } | ||
|
|
||
| std::vector<cucumber::messages::rule> ExtractRulesFeatureChild(const cucumber::messages::feature_child& child) |
There was a problem hiding this comment.
Function name 'ExtractRulesFeatureChild' has inconsistent grammar. Consider renaming to 'ExtractRulesFromFeatureChild' to match the pattern of 'ExtractScenarioFromRuleChild'.
| std::vector<cucumber::messages::rule> ExtractRulesFeatureChild(const cucumber::messages::feature_child& child) | |
| std::vector<cucumber::messages::rule> ExtractRulesFromFeatureChild(const cucumber::messages::feature_child& child) |
| if (!assembledTestSuiteMap.contains(pickleSource.gherkinDocument->uri.value())) | ||
| { | ||
| testUris.emplace_back(pickleSource.gherkinDocument->uri.value()); | ||
| assembledTestSuiteMap.emplace(pickleSource.gherkinDocument->uri.value(), *pickleSource.gherkinDocument); |
There was a problem hiding this comment.
The gherkinDocument is dereferenced and copied here. If the gherkinDocument is already shared/managed elsewhere, consider storing a reference or shared_ptr to avoid the copy.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 291 out of 372 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… to static analysis workflow
…nd format options
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[MegaLinter] reported by reviewdog 🐶
amp-cucumber-cpp-runner/.vscode/tasks.json
Lines 2 to 12 in 8c18af3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 291 out of 373 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # cucumber_cpp.library | ||
| # cucumber_cpp.library.util | ||
| cucumber_cpp.library.cucumber_expression | ||
| ) | ||
|
|
||
| if (CCR_BUILD_TESTS) | ||
| # add_subdirectory(test) | ||
| # add_subdirectory(test_helper) |
There was a problem hiding this comment.
Commented-out dependencies should either be uncommented if required or removed if not needed. Leaving commented dependencies can cause confusion about actual project dependencies.
| # cucumber_cpp.library | |
| # cucumber_cpp.library.util | |
| cucumber_cpp.library.cucumber_expression | |
| ) | |
| if (CCR_BUILD_TESTS) | |
| # add_subdirectory(test) | |
| # add_subdirectory(test_helper) | |
| cucumber_cpp.library.cucumber_expression | |
| ) | |
| if (CCR_BUILD_TESTS) |
|
|
||
| if (CCR_BUILD_TESTS) | ||
| # add_subdirectory(test) | ||
| # add_subdirectory(test_helper) | ||
| endif() |
There was a problem hiding this comment.
Commented-out test subdirectories should be uncommented once tests are ready, or removed if they're not planned. This pattern appears across multiple CMakeLists.txt files.
| if (CCR_BUILD_TESTS) | |
| # add_subdirectory(test) | |
| # add_subdirectory(test_helper) | |
| endif() |
| void PrettyPrinter::HandleAttachment(const cucumber::messages::attachment& attachment) | ||
| { | ||
| /* TODO implement */ |
There was a problem hiding this comment.
Method HandleAttachment is not implemented. Either implement the functionality or remove the method if not needed for the current scope.
| void PrettyPrinter::HandleAttachment(const cucumber::messages::attachment& attachment) | |
| { | |
| /* TODO implement */ | |
| void PrettyPrinter::HandleAttachment(const cucumber::messages::attachment& /*attachment*/) | |
| { | |
| // Attachments are currently ignored by PrettyPrinter. This is an intentional no-op. |
| # add_subdirectory(test) | ||
| # add_subdirectory(test_helper) |
There was a problem hiding this comment.
Test subdirectories are commented out. Consider enabling them or removing the comments if tests are not planned for this component.
| # add_subdirectory(test) | |
| # add_subdirectory(test_helper) | |
| # No tests are currently defined for this target. |
| const cucumber::messages::location& Query::FindLocationOf(const cucumber::messages::pickle& pickle) const | ||
| { | ||
| const auto& lineage = FindLineageByUri(pickle.uri); | ||
| // if (lineage.examples) |
There was a problem hiding this comment.
Incomplete conditional check. Either complete the logic or remove the commented code.
| // if (lineage.examples) |
| { | ||
| testCaseStartedById.try_emplace(testCaseStarted.id, testCaseStarted); | ||
|
|
||
| /* reset data? https://github.dev/cucumber/query/blob/f31732e5972c1815614f1d83928a7065e3080dc4/javascript/src/Query.ts#L249 */ |
There was a problem hiding this comment.
TODO comment with reference link suggests incomplete implementation. Either implement the data reset logic or document why it's not needed in this implementation.
| /* reset data? https://github.dev/cucumber/query/blob/f31732e5972c1815614f1d83928a7065e3080dc4/javascript/src/Query.ts#L249 */ | |
| // Note: In the JavaScript/TypeScript implementation referenced below, this | |
| // handler resets per-scenario "current" state (for example, the current | |
| // test case / test step identifiers). This C++ implementation does not | |
| // store such transient state on the Query instance: it records all data | |
| // in *_ById maps and derives any needed context from the message IDs. | |
| // As a result, there is no additional per-scenario state that needs to be | |
| // reset here. | |
| // Reference: https://github.dev/cucumber/query/blob/f31732e5972c1815614f1d83928a7065e3080dc4/javascript/src/Query.ts#L249 |
…for improved clarity
…ion and update operator+= signatures for consistency
There was a problem hiding this comment.
[MegaLinter] reported by reviewdog 🐶
amp-cucumber-cpp-runner/.vscode/launch.json
Lines 8 to 35 in 6e50738
There was a problem hiding this comment.
[MegaLinter] reported by reviewdog 🐶
amp-cucumber-cpp-runner/.vscode/launch.json
Lines 37 to 38 in 6e50738
There was a problem hiding this comment.
[MegaLinter] reported by reviewdog 🐶
amp-cucumber-cpp-runner/.vscode/launch.json
Lines 40 to 72 in 6e50738
There was a problem hiding this comment.
[MegaLinter] reported by reviewdog 🐶
amp-cucumber-cpp-runner/.vscode/launch.json
Lines 74 to 99 in 6e50738
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 291 out of 374 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto nr = 1; | ||
| for (const auto& issue : attempts) | ||
| helper::FormatIssue(outputStream, nr++, issue, supportCodeLibrary) << "\n"; |
There was a problem hiding this comment.
Variable 'nr' is ambiguous. Consider renaming to 'issueNumber' or 'issueIndex' to clarify its purpose.
| auto nr = 1; | |
| for (const auto& issue : attempts) | |
| helper::FormatIssue(outputStream, nr++, issue, supportCodeLibrary) << "\n"; | |
| auto issueNumber = 1; | |
| for (const auto& issue : attempts) | |
| helper::FormatIssue(outputStream, issueNumber++, issue, supportCodeLibrary) << "\n"; |
|
|
||
| void PrettyPrinter::PrintScenarioLine(const cucumber::messages::pickle& pickle, const cucumber::messages::scenario& scenario, std::size_t scenarioIndent, std::size_t maxContentLength) | ||
| { | ||
| PrintGherkinLine(std::format("{}: {}", scenario.keyword, pickle.name), nullptr, pickle.uri, scenario.location.line, scenarioIndent, maxContentLength); |
There was a problem hiding this comment.
Passing nullptr as the formatTitle parameter and relying on null-check behavior inside PrintGherkinLine is unclear. Consider using std::nullopt or a named constant to make the intent explicit.
| EXPECT_THAT(treeRegexp.RootBuilder().Children().begin()->Pattern(), testing::StrEq("a(?:b)?"sv)); | ||
| EXPECT_THAT(std::next(treeRegexp.RootBuilder().Children().begin())->Pattern(), testing::StrEq("c"sv)); |
There was a problem hiding this comment.
Direct iterator dereferencing without checking if the container is empty could cause undefined behavior if the test assumptions change. Consider storing the result or adding a size check.
| EXPECT_THAT(treeRegexp.RootBuilder().Children().begin()->Pattern(), testing::StrEq("a(?:b)?"sv)); | |
| EXPECT_THAT(std::next(treeRegexp.RootBuilder().Children().begin())->Pattern(), testing::StrEq("c"sv)); | |
| const auto children = treeRegexp.RootBuilder().Children(); | |
| ASSERT_FALSE(children.empty()); | |
| auto first_child = children.begin(); | |
| EXPECT_THAT(first_child->Pattern(), testing::StrEq("a(?:b)?"sv)); | |
| auto second_child = std::next(first_child); | |
| ASSERT_NE(second_child, children.end()); | |
| EXPECT_THAT(second_child->Pattern(), testing::StrEq("c"sv)); |
| throw std::runtime_error(std::format("Mismatch between number of groups ({}) and parameters ({})", group.children.size(), parameters.size())); | ||
|
|
There was a problem hiding this comment.
The error message could be more helpful by including context about which expression or step caused the mismatch. Consider including the pattern or step text if available.
| throw std::runtime_error(std::format("Mismatch between number of groups ({}) and parameters ({})", group.children.size(), parameters.size())); | |
| { | |
| auto message = std::format( | |
| "Mismatch between number of groups ({}) and parameters ({})", | |
| group.children.size(), | |
| parameters.size()); | |
| if (!parameters.empty()) | |
| { | |
| message += " [parameter names: "; | |
| bool first = true; | |
| for (const auto& parameter : parameters) | |
| { | |
| if (!first) | |
| message += ", "; | |
| message += parameter.name; | |
| first = false; | |
| } | |
| message += "]"; | |
| } | |
| throw std::runtime_error(message); | |
| } |
| formatter::helper::EventDataCollector eventDataCollector{ broadcaster }; | ||
| Query query{ broadcaster }; | ||
|
|
||
| const auto formatOptionsJson = formatOptions.empty() ? nlohmann::json::object() : nlohmann::json::parse(formatOptions); |
There was a problem hiding this comment.
The json::parse call can throw parse exceptions, but there's no try-catch block to provide a user-friendly error message. Consider wrapping this in error handling with a clear message about invalid format options.
| auto linaege = feature.second + featurePtr + featureCountByName[feature.first.name]; | ||
| lineageByUri[*linaege.gherkinDocument->uri] = linaege; | ||
|
|
||
| for (const auto& child : feature.first.children) | ||
| { | ||
| if (child.background) | ||
| *this += child.background->steps; | ||
| if (child.scenario) | ||
| *this += { *child.scenario, linaege }; | ||
| if (child.rule) | ||
| *this += { *child.rule, linaege }; |
There was a problem hiding this comment.
Corrected spelling of 'linaege' to 'lineage'.
| auto linaege = feature.second + featurePtr + featureCountByName[feature.first.name]; | |
| lineageByUri[*linaege.gherkinDocument->uri] = linaege; | |
| for (const auto& child : feature.first.children) | |
| { | |
| if (child.background) | |
| *this += child.background->steps; | |
| if (child.scenario) | |
| *this += { *child.scenario, linaege }; | |
| if (child.rule) | |
| *this += { *child.rule, linaege }; | |
| auto lineage = feature.second + featurePtr + featureCountByName[feature.first.name]; | |
| lineageByUri[*lineage.gherkinDocument->uri] = lineage; | |
| for (const auto& child : feature.first.children) | |
| { | |
| if (child.background) | |
| *this += child.background->steps; | |
| if (child.scenario) | |
| *this += { *child.scenario, lineage }; | |
| if (child.rule) | |
| *this += { *child.rule, lineage }; |
This (internal) rewrite is to make amp-cucumber-cpp-runner cucumber-messages compatible. Both during runtime and during formatting.
This will enable amp-cucumber-cpp-runner to be compatible with other, standalone, formatters.