Skip to content

Ruby implementation - #98

Merged
luke-hill merged 58 commits into
mainfrom
feature/ruby_implementation
Jul 13, 2026
Merged

Ruby implementation#98
luke-hill merged 58 commits into
mainfrom
feature/ruby_implementation

Conversation

@luke-hill

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

Copy link
Copy Markdown
Contributor

🤔 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
Contributor 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.

@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.

Looks okay. What does the code coverage look like?

Comment thread .github/workflows/test-ruby.yml Outdated
Comment thread .github/workflows/test-ruby.yml Outdated
Comment thread ruby/lib/cucumber/query.rb Outdated
# Given one Cucumber Message, find another.
#
# Queries can be made while the test run is incomplete - and this will naturally return incomplete results
# see <a href="https://github.com/cucumber/messages?tab=readme-ov-file#message-overview">Cucumber Messages - Message Overview</a>

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/lib/cucumber/query.rb
Comment thread ruby/lib/cucumber/query.rb Outdated
find_test_steps_finished_by(message)
.map(&:test_step_result)
.max_by { |test_step_result| test_step_result_rankings[test_step_result.status] }
# Java code: "PREVIOUS".max(comparing(TestStepResult::getStatus, new TestStepResultStatusComparator()));

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.

Comments like this can be cleaned up.

Comment thread ruby/lib/cucumber/query.rb Outdated

def message_types
{
pickle_step: Cucumber::Messages::PickleStep,

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.

Does Ruby have some sort of alias that lets you use the qualified name instead of the unqualified name? If so, this lookup table could be removed.

attachments_by_test_run_hook_started_id[attachment.test_run_hook_started_id] << attachment if attachment.test_run_hook_started_id
end

def update_feature(feature)

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.

I would suggest removing this method and it's dependencies for now. You won't need it unless you implement the naming strategy which is a long way away. It's also not testable without implementing the naming strategy so you can safe on some work here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update @mpkorstanje we need this method and "some" type of handler (Even if it's a nil handler), to handle the metaprogrammed enumeration.

We've added in 2 more "dumb" handlers for undefined_parameter_type and source as well as a catchall method that'll bubble up a warning that things aren't updating.

Merging this in AS IS, as it's essentially RtM - but there's no need to rush anything out for this, so it can just sit until someone builds a new version. 16.1+

@mpkorstanje mpkorstanje Jul 13, 2026

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.

Then why not use a nil handler? Dead and untested code just slow you down.

@luke-hill

Copy link
Copy Markdown
Contributor Author

Note to viewers. CI is red whilst we move forward on messages being cut for a new major - Which would bring version parity in-line plus enable access to new methods

@luke-hill

Copy link
Copy Markdown
Contributor Author

Updated with latest main and fixed version requirements to pick up a new enough version of messages to facilitate this working.

Should be green now

@luke-hill
luke-hill merged commit cc25e82 into main Jul 13, 2026
19 checks passed
@luke-hill
luke-hill deleted the feature/ruby_implementation branch July 13, 2026 13:42
@mpkorstanje
mpkorstanje self-requested a review July 13, 2026 13:42
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