(dotnet) Migrate gherkin messages to cucumber messages

[clrudolphi]

🤔 What's changed?

This PR enhances the .Net implementation such that the Message events and pickles that are generated use the Cucumber/Messages types instead of the Gherkin.Cucumber.Messages.Types which had been embedded in the Gherkin project.

⚡️ What's your motivation?

Parity with other implementation languages.
This will also simplify the Reqnroll implementation of Messages.

🏷️ What kind of change is this?

• 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
• 💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

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

[clrudolphi]

Tests run locally are successful. I'm not sure, from reading the build log, why the test-dotnet job failed. Can anyone help me out?

[mpkorstanje]

I'm not sure, from reading the build log, why the test-dotnet job failed. Can anyone help me out?

bin/gherkin --no-source --no-pickles ../testdata/good/i18n_fr.feature | jq --sort-keys --compact-output "." > acceptance/testdata/good/i18n_fr.feature.ast.ndjson

Produced an AST with "keywordType": "CONTEXT" instead of the expected keywordType": "Context".

Is there perhaps a conflict between https://github.com/cucumber/gherkin/blob/main/dotnet/Gherkin/StepKeywordType.cs and https://github.com/cucumber/messages/blob/main/dotnet/Cucumber.Messages/generated/StepKeywordType.cs with the json deserializer only picking up the instructions for the enum from messages?

Why this only happens in the french feature files is indeed a bit puzzling.

[mpkorstanje]

Some remarks see below. I don't see anything major.

[mpkorstanje]

Why this only happens in the french feature files is indeed a bit puzzling.

Without looking further into the details, I think that might be another bug actually. Something along the lines of #400.

I think that if you try to replace the StepKeywordType in Gherkin with the one from messages you'll find your issue.

Tests run locally are successful

And this is probably because some lookup is stored in a map or set with non-determistic ordering.

[mpkorstanje]

On inspection this looks suspicious:

gherkin/dotnet/Gherkin/GherkinDialect.cs

Lines 40 to 49 in c06186f

	public IDictionary<string, StepKeywordType> StepKeywordTypes { get; } = new[] { new { Keyword = AsteriskKeyword, Type = StepKeywordType.Unknown } }
	.Concat(givenStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Context }))
	.Concat(whenStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Action }))
	.Concat(thenStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Outcome }))
	.Concat(andStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Conjunction }))
	.Concat(butStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Conjunction }))
	.GroupBy(item => item.Keyword, item => item.Type)
	.ToDictionary(item => item.Key, item => item.First());

By using .ToDictionary(item => item.Key, item => item.First()); it assumes that the AsteriskKeyword is the only duplicate keyword. Which probably isn't true. This should be something like item => item.Size() == 1 ? Unknown : item.First();

Again apologies for any vagueness. I can't actually run this code locally.

[clrudolphi]

Why this only happens in the french feature files is indeed a bit puzzling.

Without looking further into the details, I think that might be another bug actually. Something along the lines of #400.

I think that if you try to replace the StepKeywordType in Gherkin with the one from messages you'll find your issue.

Tests run locally are successful

And this is probably because some lookup is stored in a map or set with non-determistic ordering.

Actually the cause is simple, but not sure of the path forward.
This fails on the french feature file b/c this is the first feature file in the test script for which the test suite is attempting to compare the generated GherkinDocument. All prior tests in the suite are comparing the tokens. Therefore, this is the first diff test that is looking at the output that includes Messages types (and enum values). The acceptance test files use the old StepKeywordType enumeration, but the revised code uses the StepKeywordType enum from Messages (which accounts for the difference in casing in the values). The test run stops after the first failure, otherwise we would have seen the other ast, source and pickle acceptance tests also failing for the same reason.

My doubt however is why this hasn't surfaced for the other implementation languages that have already adopted Messages. Their tests too should show a difference between what is output and what the acceptance files have as content.

[clrudolphi]

I had not tried previously to regenerate the .net parser, so I attempted that by following the directions on the Contributing.MD page (spinning up a docker container and running the make commands).
That is failing b/c the dockerfile was auto-updated last week to use .net9 while berp needs .net8.

Was that a mistake?

[mpkorstanje]

Therefore, this is the first diff test that is looking at the output that includes Messages types (and enum values). The acceptance test files use the old StepKeywordType enumeration, but the revised code uses the StepKeywordType enum from Messages (which accounts for the difference in casing in the value

This PR writes the uppercased enum names. The expected value is the pascal cased description on the enum constant. So presumably the serialisation isn't done correctly. The other implementations do seem to do this correctly.

https://github.com/cucumber/messages/blob/ad48503f9ff91d5e249928c833a90aa4326ed481/dotnet/Cucumber.Messages/generated/StepKeywordType.cs#L18-L19

That is failing b/c the dockerfile was auto-updated last week to use .net9 while berp needs .net8.

Did you try to downgrade that again?

[clrudolphi]

I'm not sure, from reading the build log, why the test-dotnet job failed. Can anyone help me out?

bin/gherkin --no-source --no-pickles ../testdata/good/i18n_fr.feature | jq --sort-keys --compact-output "." > acceptance/testdata/good/i18n_fr.feature.ast.ndjson

Produced an AST with "keywordType": "CONTEXT" instead of the expected keywordType": "Context".

I found the problem within the CLI section of the testing project. It was using a default enum converter. I've changed that to use the proper one for the Messages enums. All tests pass now.

[mpkorstanje]

I had not tried previously to regenerate the .net parser, so I attempted that by following the directions on the Contributing.MD page (spinning up a docker container and running the make commands).
That is failing b/c the dockerfile was auto-updated last week to use .net9 while berp needs .net8.

Was that a mistake?

More a a lack of test coverage. Renovate proposed several updates.

• The benchmark module isn't executed in CI. So pipelines were green and updates got merged.
• The Docker file isn't used in CI. So pipelines were green and updates got merged.

Fixed with 34af477.

[mpkorstanje]

Looks good to me. But I assume you'll want to fix the StepKeywordType? If so I'll hold off on merging until that is done.

[clrudolphi]

On inspection this looks suspicious:

gherkin/dotnet/Gherkin/GherkinDialect.cs

Lines 40 to 49 in c06186f

	public IDictionary<string, StepKeywordType> StepKeywordTypes { get; } = new[] { new { Keyword = AsteriskKeyword, Type = StepKeywordType.Unknown } }
	.Concat(givenStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Context }))
	.Concat(whenStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Action }))
	.Concat(thenStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Outcome }))
	.Concat(andStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Conjunction }))
	.Concat(butStepKeywords.Select(kw => new { Keyword = kw, Type = StepKeywordType.Conjunction }))
	.GroupBy(item => item.Keyword, item => item.Type)
	.ToDictionary(item => item.Key, item => item.First());

By using .ToDictionary(item => item.Key, item => item.First()); it assumes that the AsteriskKeyword is the only duplicate keyword. Which probably isn't true. This should be something like item => item.Size() == 1 ? Unknown : item.First();

Again apologies for any vagueness. I can't actually run this code locally.

I haven't looked into this, but consider it out of scope for this PR. This is within the parser and not anything I'm touching right now.

[clrudolphi]

@mpkorstanje I've updated the handling of StepKeywordType as I understand it should be done. Please reconfirm.
Gaspar will return from holiday next week. Let's give him a chance to review this before we merge. Thanks!

[mpkorstanje]

The handling of StepKeywordType looks good. Merged #427 into your branch and that passes CI on #434. Can you pull #427 into your branch?

[mpkorstanje]

I haven't looked into this, but consider it out of scope for this PR. This is within the parser and not anything I'm touching right now.

No problem. Made #435 to address that.

[clrudolphi]

The handling of StepKeywordType looks good. Merged #427 into your branch and that passes CI on #434. Can you pull #427 into your branch?

Completed.