-
Notifications
You must be signed in to change notification settings - Fork 30
Refactor conformance test case ingestion #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor conformance test case ingestion #131
Conversation
stefanvanburen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't pretend to have fully reviewed or understand the existing gherkin process, but what I did see looked good to me. just a couple of passing nits, really
51e0072 to
3839ec5
Compare
slott56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very, very nice. Thoughtful, careful, and wonderfully complete. I'm delighted with the resulting simplification to managing conformance testing with the baseline CEL-spec. This is a super-helpful foundation.
Two things:
-
Can we switch the
wip.txtto be awip.tomlso that it can be enriched with other attributes, like plans, status, dates, etc.? (Maybe this is too much work for only a potential value.) -
Can we add a module-level docstring at the top of the new gherkinize.py. This would help create a unified Sphinx-friendly source for internal documentation.
Feel free to explain to me why these might be bad ideas.
12952df to
1122865
Compare
1122865 to
81f5894
Compare
slott56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure of the TOML for WIP tags looks really, really helpful. It parallels the source and leaves plenty of room to add notes or other fields and attributes and things.
I'm not convinced the unique classes are helpful. I would like some docstring comments to provide a few sentences on why this structure is helpful. Especially because toml already produces a nested dict[str, dict[str, dict[str, tags]]] structure that we're replacing with something almost but not quite the same.
9829119 to
a4d8844
Compare
slott56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent docstring for the new app. The configuration reflects an inherent complexity in the problem domain and a couple of comments explain this.
(Most important. It eliminates the legacy, clumsy, multi-step mess.)
tools/gherkinize.py)tools/wip.txtlist to annotate individual conformance test scenarios with@wiptools/wip.txtwith currently-failing teststools/README.rstto describe the new tooling.github/workflows/ci.yamlto useuv, as the rest of the repo does (I needed to do this so I wouldn't have to get the new build dependencies working in two separate systems)Makefileandfeatures/Makefileto remove obsolete targets and add more granularity for testingfeatures/steps/integration_binding.pyto remove workarounds no longer needed for the cleaner, more consistent output of the new script.I recommend starting with the new
tools/README.rst.Apologies for the large change set, but I have tried to make the changes as reviewable as I could:
features/*.featureare VERY similar to the old output.src/has only been touched to tweak some linter annotations — this PR does not touch the runtime, and this is very intentional!test_pb2g.pyto the newtest_gherkinize.pyas a way of providing some confidence that the new test generation path considers everything the old one did. Unfortunately GitHub does not show this as a rename, so if you want to see them side-by-side you'll have to pull them down and do a diff (I recommend running an aggressive formatter on both first).