[messages - elixir] Convert proto-based messages to json schema based messages #1952

WannesFransen1994 · 2022-04-14T13:22:42Z

Summary

Remove protobuf dependency and create modules based on json schema (manual parser). No validation of data in messages, that's done through acceptance testing.

Checklist:

Clean dependencies
Generate modules in generated folder
Adjust Make file & do integrations in CICD
See if compatibility testing kit is relevant
Adjust gherkin library as well and verify with acceptance testing there
Provide meaningful debug messages if applicable

…. Just forward to default json encoder

…ew messages. Still runtime issues considering some struct values are by default nil instead of empty list

WannesFransen1994 · 2022-05-20T11:24:40Z

Attempted a makefile adjustment, but seems that there's a problem with the messages generation. (based on the log)

==> cucumber_messages
Compiling 3 files (.ex)
Generated cucumber_messages app

…e's no longer a "synchronization" script in place)

WannesFransen1994 · 2022-05-23T09:10:40Z

Not that familiar with the whole monorepo build, but I've managed to make it work with adjusting the Makefile. Would love some pointers / reviews on the integration, specifically for

manually copying the json schema files
instead of adjusting the general gitignore - which caused an rsync conflict-, I've adjusted an ignore file here
The CircleCI config integration file
Instead of running the compiled binary with every file and verifying the output, I do this in the test suite (since starting an beam VM every time is expensive). Saves a lot of time though (would be even more if we ran this in parallel, though not sure it is worth the effort)

ehuelsmann · 2022-06-19T20:13:43Z

gherkin/elixir/lib/gherkin/ast_builder/ast_builder.ex

-  alias CucumberMessages.GherkinDocument.Feature.TableRow.TableCell, as: TableCellMessage
+  alias CucumberMessages.GherkinDocument.Tag, as: MessageTag
+  alias CucumberMessages.GherkinDocument.Scenario, as: MessageScenario
+  alias CucumberMessages.GherkinDocument.Step, as: StepMessage


@WannesFransen1994 is this document manually maintained? I see an inconsistency here: StepMessage where all the others are called Message*?

ehuelsmann · 2022-06-19T20:16:06Z

gherkin/elixir/lib/gherkin/ast_builder/ast_builder.ex

@@ -58,7 +58,15 @@ defmodule CucumberGherkin.AstBuilder do
      Comment ->
        loc = Token.get_location(token)
        comment_message = %CommentMessage{location: loc, text: token.line.content}
+        # TODO: Normally your struct should default to an empty list instead of nil.
+        #   Due to the limited converter in the messages library, we make a case clause and "catch" this unexpected `nil` value.


Is there an option to make the converter smarter? It's nice not to have to maintain this workaround "forever".

ehuelsmann · 2022-06-19T20:19:04Z

gherkin/elixir/lib/gherkin/ast_builder/ast_builder.ex

+      # case feature_message_acc.children do
+      #   nil -> %{feature_message_acc | children: [child]}
+      #   list -> %{feature_message_acc | children: list ++ [child]}
+      # end


I'm actually not seeing the case statement; I'm not all that great at elixir, but the code you describe really isn't there?

ehuelsmann · 2022-06-19T20:21:21Z

gherkin/elixir/lib/gherkin/pickle_compiler/pickle_compiler.ex

-  alias CucumberMessages.GherkinDocument.Feature.TableRow, as: TableRowMessage
+  alias CucumberMessages.GherkinDocument.Scenario, as: ScenarioMessage
+  alias CucumberMessages.GherkinDocument.Step, as: StepMessage
+  alias CucumberMessages.GherkinDocument.TableRow, as: TableRowMessage


These are inconsistent with ast_builder.ex where you use Message as a prefix. I think it's easier to understand the code base if you stick to the same naming rules. Or is there a specific reason to deviate here?

ehuelsmann · 2022-06-19T20:24:12Z

gherkin/elixir/lib/gherkin/pickle_compiler/pickle_compiler.ex

+
+      #   {:scenario, s} ->
+      #     compile_scenario(m_acc, s, :feature_backgr_steps)
+      # end


What's the value of this comment block? It looks like it's preserving the old code, but once we're moved over, we have the old code in Git for historic reference. No need to keep a copy?

ehuelsmann · 2022-06-19T20:40:09Z

messages/elixir/Makefile

 	rm -rf lib/cucumber_messages/generated/*

-.deps: setup_mix_and_get_dependencies update_proto_file compile_messages revert_proto_file
+# .deps: setup_mix_and_get_dependencies update_proto_file compile_messages revert_proto_file


Remnant can be removed?

ehuelsmann · 2022-06-19T20:40:29Z

messages/elixir/Makefile

-	cat $<.bak | sed "s/package io.cucumber.messages/package cucumber_messages/" > $<
+# update_proto_file: messages.proto
+# 	mv $< $<.bak
+# 	cat $<.bak | sed "s/package io.cucumber.messages/package cucumber_messages/" > $<


Remnant can be removed?

ehuelsmann · 2022-06-19T20:41:07Z

messages/elixir/Makefile

-.PHONY: revert_proto_file
+# revert_proto_file: messages.proto.bak
+# 	mv messages.proto.bak messages.proto
+# .PHONY: revert_proto_file


Remnant can be removed?

ehuelsmann · 2022-06-19T20:44:17Z

messages/elixir/lib/cucumber_messages/mix/tasks/generate_json_structs.ex

+    # metadata = %{definitions: nil, full_decoded: decoded, modules: [], parent: nil}
+    # list_of_modules = create_moduledata_structs(metadata, decoded)
+    # textual_modules = output_module(list_of_modules, [])
+    # write_files(textual_modules)


Remnant that can be removed?

ehuelsmann · 2022-06-19T20:49:22Z

messages/elixir/lib/cucumber_messages/mix/tasks/generate_json_structs.ex

+        # case create_moduledata_structs(empty_metadata, {name, child_member}) do
+        #   %ModuleData{} = submodule -> submodule
+        #   other -> raise "unexpected"
+        # end


Was this a remnant?

aurelien-reeves

We pair today with @WannesFransen1994 to review the PR.
We noticed the following:

Please join the generated code for messages as part of the repo
We usually use the CCK as part of the messages high-level validation to make sure they serialize and deserialize as expected (you can take inspiration from ruby, perl, php, java or js implementation for that)
If you want to comply with request from Erik, you can copy the good/bad test data using make but keeping those ignored in the repo

Beside that: great job @WannesFransen1994! 👍Thanks a lot!

aurelien-reeves · 2022-06-17T08:32:09Z

messages/elixir/Makefile

+# update_proto_file: messages.proto
+# 	mv $< $<.bak
+# 	cat $<.bak | sed "s/package io.cucumber.messages/package cucumber_messages/" > $<


Suggested change

# update_proto_file: messages.proto

# mv $< $<.bak

# cat $<.bak | sed "s/package io.cucumber.messages/package cucumber_messages/" > $<

aurelien-reeves · 2022-08-17T07:44:29Z

PR now available in the dedicated repo here: cucumber/messages#29

WannesFransen1994 added 2 commits April 14, 2022 15:17

[messages - elixir] create proof of concept writer for module generation

91f826a

[messages - elixir] remove unused dependencies in

4173fdc

WannesFransen1994 self-assigned this Apr 14, 2022

WannesFransen1994 added library: cucumber-messages json-to-messages language: elixir labels Apr 14, 2022

Merge branch 'main' into messages-elixir-proto-to-jsonschema

bf28218

WannesFransen1994 mentioned this pull request Apr 14, 2022

Migrate elixir from protobuf to JSON Schema based messages cucumber/messages#63

Open

WannesFransen1994 added 4 commits April 14, 2022 15:44

[messages - elixir] remove unnecessary compile time struct dependency…

080b499

…. Just forward to default json encoder

[messages-elixir + gherkin-elixir] resolve compile-time issues with n…

1c57e00

…ew messages. Still runtime issues considering some struct values are by default nil instead of empty list

adjusted gherkin parser so that tests pass

6864a7c

[elixir] Makefile + circleci conf attempt

bf9e23f

WannesFransen1994 added 6 commits May 20, 2022 13:27

Merge branch 'main' into messages-elixir-proto-to-jsonschema

a1ac0e0

[elixir] merge + implement unsynced testdata

fd21557

[elixir] remove debugging code in tests

3e922b1

[elixir messages] manually copy json schema files (assuming that ther…

6007042

…e's no longer a "synchronization" script in place)

[elixir] attempt to avoid rsync conflict

c305c72

Merge branch 'main' into messages-elixir-proto-to-jsonschema

176bd7e

WannesFransen1994 marked this pull request as ready for review May 23, 2022 09:47

Merge branch 'main' into messages-elixir-proto-to-jsonschema

a703c12

ehuelsmann reviewed Jun 19, 2022

View reviewed changes

aurelien-reeves suggested changes Jun 21, 2022

View reviewed changes

aurelien-reeves mentioned this pull request Jul 1, 2022

Move messages to stand-alone repo #2029

Closed

aurelien-reeves mentioned this pull request Aug 17, 2022

[elixir] Convert proto-based messages to json schema based messages cucumber/messages#29

Open

aurelien-reeves closed this Aug 17, 2022

luke-hill deleted the messages-elixir-proto-to-jsonschema branch August 15, 2023 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[messages - elixir] Convert proto-based messages to json schema based messages #1952

[messages - elixir] Convert proto-based messages to json schema based messages #1952

WannesFransen1994 commented Apr 14, 2022 •

edited

Loading

WannesFransen1994 commented May 20, 2022

WannesFransen1994 commented May 23, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

ehuelsmann Jun 19, 2022

aurelien-reeves left a comment

aurelien-reeves Jun 17, 2022

aurelien-reeves commented Aug 17, 2022

	# update_proto_file: messages.proto
	# mv $< $<.bak
	# cat $<.bak \| sed "s/package io.cucumber.messages/package cucumber_messages/" > $<

[messages - elixir] Convert proto-based messages to json schema based messages #1952

[messages - elixir] Convert proto-based messages to json schema based messages #1952

Conversation

WannesFransen1994 commented Apr 14, 2022 • edited Loading

Summary

Checklist:

WannesFransen1994 commented May 20, 2022

WannesFransen1994 commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurelien-reeves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurelien-reeves commented Aug 17, 2022

WannesFransen1994 commented Apr 14, 2022 •

edited

Loading