Skip to content

Ruby implementation#98

Open
luke-hill wants to merge 35 commits into
mainfrom
feature/ruby_implementation
Open

Ruby implementation#98
luke-hill wants to merge 35 commits into
mainfrom
feature/ruby_implementation

Conversation

@luke-hill

@luke-hill luke-hill commented Sep 5, 2025

Copy link
Copy Markdown

🤔 What's changed?

Just enough cucumber-query to satisfy the rerun formatter.

⚡️ What's your motivation?

Move code that was written inside Cucumber-ruby over to this library. Get a first release out that we can build on.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

Are we following standard patterns? Is this a good foundation to build on?

Checklist

  • Configure RubyGems trusted publishing for cucumber-query
  • Do the release

@mpkorstanje mpkorstanje left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you're writing tests for each method. That isn't necessary.

Rather you can define a list of sources:

  • "../testdata/attachments.ndjson"
  • "../testdata/empty.ndjson"
  • "../testdata/hooks.ndjson"
  • "../testdata/minimal.ndjson"
  • "../testdata/rules.ndjson"
  • "../testdata/examples-tables.ndjson"

And a map of query functions queries of the form (Query query) -> /* execute some query and return the results*/. For example:

queries.put("findAllTestStepsStarted", (query) -> query.findAllTestStepsStarted().size());

Then take the product of sources and queries and for each of those populate a instance of the Query object with the messages from the source, and then apply the query function to the Query object.

Then you compare the result of query function to the ../testdata/<source>.<methodname>.results.json file.

The nice thing is that you can add methods to the queries list as you implement them.

@luke-hill

Copy link
Copy Markdown
Author

Atm I'm copying over things that already exist. I'm well aware this is likely wrong. I just want to get what currently "works" in here and equivalent passing/failing.

See https://github.com/cucumber/cucumber-ruby/tree/main/lib/cucumber/formatter/query for where I'm adapting things from for now.

@mpkorstanje mpkorstanje left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other nitpicks below, but I suppose those are there because this is WIP

Comment thread ruby/cucumber-query.gemspec Outdated

s.add_dependency 'cucumber-messages', '> 25', '< 30'

s.add_development_dependency 'cucumber', '~> 10.1'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment thread ruby/cucumber-query.gemspec
Comment thread ruby/cucumber-query.gemspec Outdated
s.metadata = {
'bug_tracker_uri' => 'https://github.com/cucumber/query/issues',
'changelog_uri' => 'https://github.com/cucumber/query/blob/main/CHANGELOG.md',
'documentation_uri' => 'https://github.com/cucumber/query/blob/main/CONTRIBUTING.md',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be the read me I guess, not the contributors guide.

def hook_id(test_step)
return @hook_id_by_test_step_id[test_step.id] if @hook_id_by_test_step_id.key?(test_step.id)

raise TestStepUnknownError, "No hook found for #{test_step.id} }. Known: #{@hook_id_by_test_step_id.keys}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design of the query object assumes that if something can't be found either an empty list, empty optional or null is returned. There are no methods that throw.

This is also why all the methods are name findX or findAllX or findZByX`.

@mpkorstanje

Copy link
Copy Markdown
Member

Atm I'm copying over things that already exist. I'm well aware this is likely wrong. I just want to get what currently "works" in here and equivalent passing/failing.

That doesn't seem like a good idea. The query object is used by the xml, json, pretty formatters, ect. By using a different API the Ruby implementations of these will lose the resemblance to the Java and Javascript versions. That will make maintenance much harder.

I think you'd be better of leaving that old query object in place, implement the new query object here, then migrate your users of the old query object over to the new query object instead.

@mpkorstanje mpkorstanje marked this pull request as draft September 5, 2025 18:27
@mpkorstanje mpkorstanje changed the title WIP: Ruby implementation Ruby implementation Sep 5, 2025
@mpkorstanje

Copy link
Copy Markdown
Member

Removed WIP from title, marked PR as draft.

@mattwynne mattwynne force-pushed the feature/ruby_implementation branch from eeb238d to 4c70e37 Compare June 13, 2026 20:42
Comment thread .github/workflows/release-rubygem.yaml Fixed
Comment thread .github/workflows/release-rubygem.yaml Fixed
Comment thread .github/workflows/release-rubygem.yaml Fixed
Comment thread .github/workflows/release-rubygem.yaml Fixed
Comment thread .github/workflows/release-rubygem.yaml Fixed
Comment thread .github/workflows/test-ruby.yml Fixed
Comment thread .github/workflows/test-ruby.yml Fixed
Comment thread .github/workflows/release-rubygem.yaml Fixed
@mattwynne mattwynne marked this pull request as ready for review June 13, 2026 21:43

@luke-hill luke-hill left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12/18 files marked as good. Few minor comments

Comment thread .github/workflows/test-ruby.yml Outdated
ruby: ['3.2', '3.3', '3.4', '3.5']
include:
- os: macos-latest
ruby: '3.4'

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4.0 here and for all single ruby variants

Comment thread ruby/Gemfile.lock
@@ -0,0 +1,75 @@
PATH

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file needs gitignoring

Comment thread ruby/VERSION
@@ -0,0 +1 @@
15.0.1

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This numbers seems wrong. But I believe this shouldn't matter @mpkorstanje as it'll be overwritten on the next polyglot-release we call?

@luke-hill luke-hill left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items previously mentioned for review @mpkorstanje


# This method will be called with 1 of these 2 messages
# [TestCaseStarted || TestCaseFinished]
def find_most_severe_test_step_result_by(message)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpkorstanje - Method 1 to review as/when you have time


# This method will be called with 1 of these 5 messages
# [TestCase || TestCaseStarted || TestCaseFinished || TestStepStarted || TestStepFinished]
def find_pickle_by(message)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpkorstanje - Method 2 to review as/when you have time

end
end

def update_gherkin_document(gherkin_document)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpkorstanje this is the only part of the repository update I think you need to review.

We previously spoke of Lineages but I think this is now considered good.

}.freeze
private_constant :UPDATE_HANDLERS

def update(envelope)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self / @mattwynne

I'm proposing to write a small amount of custom code inside the Envelope message that'll return a type. Which should then make this a bit simpler/less metaprogrammy

repository.test_case_finished_by_test_case_started_id[test_case_started.id]
end

def find_test_run_duration

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpkorstanje - This one I think can be ignored for review, but I had it flagged. Would be good to know if we have some edge case specs for this where the missing values are there.

@mpkorstanje mpkorstanje self-requested a review June 16, 2026 15:11
Co-authored-by: Luke Hill <20105237+luke-hill@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants