Skip to content

Conversation

@clrudolphi
Copy link

@clrudolphi clrudolphi commented Jul 8, 2025

🤔 What's changed?

Adding a dotnet implementation. This is more or less a direct port of the Java source.

⚡️ What's your motivation?

Will be using this to implement/enhance the conformation testing of the Reqnroll implementation of Messages.

🏷️ What kind of change is this?

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

♻️ Anything particular you want feedback on?

I need a review of the QueryTest suite to ensure that I have all the tests included.

This needs integration into the CI pipeline.
This will need a nuget push as well.

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@mpkorstanje mpkorstanje self-assigned this Jul 9, 2025
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

I need a review of the QueryTest suite to ensure that I have all the tests included.

If you use the data from the testdata folder, that should be guaranteed.

Will be using this to implement/enhance the conformation testing of the Reqnroll implementation of Messages.

I'm not sure if the query library is the right tool for that. It doesn't provide a systematic list of operations on messages yet. Rather it is driven by the needs of the various report generators that we're migrating to messages. So I'm afraid we might end up with rather disjoint libraries if it used by one implementation for conformance testing and for reporting in another.

The implementation looks good over all. A few remarks:

  • The access modifiers seem to be a bit to open. I'd like to keep the public API of these libraries small and well defined. This makes it easier to create new versions without breaking changes and if we do make breaking changes, communicate those through the version number.

  • There is a little bit of technical debt that I think would be good to clean up from Java first before we duplicate it to .Net.

- Updated `Lineage` class to implement `IEquatable<Lineage>`, adding a specific `Equals` method and `GetHashCode`.

- Simplified timestamp conversion in `Query` class using `ToDateTimeOffset()` extension method.
- Changed `TimestampComparer` visibility from public to internal.
- Added `TimestampExtensions` class for converting `Timestamp` to `DateTimeOffset`.
- Minor fix in `QueryAcceptanceTest.cs` to default null names to empty strings.
@clrudolphi
Copy link
Author

I'm not sure if the query library is the right tool for that.

My intent is to use it within our Messages conformance test suite, but to use it to confirm the integrity (internal consistency) of the generated messages. Things such as a TestStepStarted points to a valid TestCaseStarted, etc.

@clrudolphi
Copy link
Author

@mpkorstanje @davidjgoss Let me know if there is anything else you'd like to see changed.
Otherwise, I'll wait for the other in-flight PRs to close and then I'll make the dotnet version match.

@mpkorstanje
Copy link
Contributor

My intent is to use it within our Messages conformance test suite, but to use it to confirm the integrity (internal consistency) of the generated messages. Things such as a TestStepStarted points to a valid TestCaseStarted, etc.

That sounds reasonable.

@mpkorstanje
Copy link
Contributor

Otherwise, I'll wait for the other in-flight PRs to close and then I'll make the dotnet version match.

Cheers! Appreciate the patience. This PR made me realize this library needed some cleanup. Otherwise we implemt composite operations multiple times.

@mpkorstanje
Copy link
Contributor

@clrudolphi I noticed that the .Net implementation of the Gherkin parser isn't using messages yet. Gaspar said he'd tackle that somewhere in Autumn. While waiting for this PR, perhaps that is something you can work on.

@clrudolphi
Copy link
Author

@clrudolphi I noticed that the .Net implementation of the Gherkin parser isn't using messages yet. Gaspar said he'd tackle that somewhere in Autumn. While waiting for this PR, perhaps that is something you can work on.

To clarify the ask, my understanding is that

  • the Astconverter and pickle compiler classes need to be changed to emit instances of Cucumber/Messages rather than the Gherkin.Cucumber.Messages.Types that are built in to the Gherkin project.
  • the Gherkin.Cucumber.Messages.Types classes should be deleted
  • the scanner and dotnet parser would NOT be changed
  • no changes required in the implementations for other platforms/languages
  • tests in the dotnet implementation would need to be updated to reflect the change in types

Please confirm this scope and correct anywhere I'm mistaken.

@mpkorstanje
Copy link
Contributor

Yes to those points with the following remarks. Please do also communicate with Gaspar to ensure the timing is convenient.

the scanner and dotnet parser would NOT be changed

Not fundamentally. There might be changes related to changing Gherkin.Cucumber.Messages.Types to Cucumber/Messages.

tests in the dotnet implementation would need to be updated to reflect the change in types

Only the unit test, if any. Unless I'm mistaken the Gherkin .Net implementation is already using the repositories acceptance tests, so the acceptance tests would not have to be changed.

And this is all on the assumption that the Cucumber/Messages and Gherkin.Cucumber.Messages.Types both serialize to the same json. But that assumption is of course not validated and may not hold. In that case, there might be a change needed to messages.

@mpkorstanje mpkorstanje self-requested a review July 15, 2025 16:29
@clrudolphi
Copy link
Author

Commit 828599e makes this match what is proposed in #84. Tests won't pass until #84 is merged (making the expected results files available).

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Sep 17, 2025

@charlierudolph I think we're at point now where we are relatively stable. More methods might be added, but the tests are setup such that we won't block each other when that happens.

@clrudolphi
Copy link
Author

Ok. I'll take another look.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me.

You can copy the test and release workflows from Gherkin https://github.com/cucumber/query/tree/main/.github/workflows.

Please sanity check them.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

LGTM. I'm going to leave this open until Monday as a reminder to release it.

@mpkorstanje mpkorstanje self-assigned this Oct 31, 2025
@clrudolphi
Copy link
Author

LGTM. I'm going to leave this open until Monday as a reminder to release it.

Could you hold off for a few days? I'd like to reconfirm package naming(re comments made by Gaspar wrt Tag-Expressions)?

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Nov 3, 2025

Not a problem! Please re-request a review when done.

@@ -0,0 +1,30 @@
using Io.Cucumber.Messages.Types;

namespace Io.Cucumber.Query
Copy link
Contributor

Choose a reason for hiding this comment

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

Await review from Gaspar on cucumber/tag-expressions#219

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.

3 participants