Skip to content

Python enable markdown acceptance tests #64

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

Merged

Conversation

temyers
Copy link
Contributor

@temyers temyers commented Nov 28, 2022

🤔 What's changed?

My version of #37

⚡️ What's your motivation?

Fix acceptance tests to support markdown in python

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)
  • ⚡ New feature (non-breaking change which adds new behaviour)
  • 💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

This: https://github.com/cucumber/gherkin/pull/64/files#r1033154806
Would appreciate a second pair of eyes on the approval tests.
These should be consistent across python and Javascript implementations (at least) (cc @aslakhellesoy)?

📋 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 changed the base branch from python-enable-markdown-acceptance-tests to main November 28, 2022 11:18
@mpkorstanje mpkorstanje marked this pull request as ready for review November 28, 2022 11:21
@mpkorstanje
Copy link
Contributor

Weird. I would have expected CI to run when I changed the target branch. I guess you'll have to push something for the build to trigger

@mpkorstanje mpkorstanje marked this pull request as draft November 28, 2022 11:22
@temyers temyers marked this pull request as ready for review December 27, 2022 09:13
@temyers
Copy link
Contributor Author

temyers commented Dec 27, 2022

@mpkorstanje
I think this is now ready for review.

I have acceptance tests passing locally (I think)

Will review any CI failures.

@temyers
Copy link
Contributor Author

temyers commented Dec 27, 2022

@mpkorstanje Ok, I think I'm blocked on this now.

TL;DR

  • There's a conflict between the python and Javascript implementations for the AST
  • The Javascript acceptance tests are dependent on an external module meaning I can't update the implementation without a release (chicken and egg situation)
  • I'm not sure how to make the Python implementation match the Javascript version - it's creating tokens I don't want, but not sure how to stop it
  • I'm not sure which implementation is more 'correct' between Javascript and Python variants.

@davidjgoss
Copy link
Contributor

@temyers if you merge main into this branch now you should at least be past the JavaScript circular dep issue.

@mpkorstanje
Copy link
Contributor

Looks like this fell of the wagon at some point. I've tried to merge main into this PR but I'm seeing more conflicts than I can solve.

Is it worth salvaging this PR?

@kieran-ryan
Copy link
Member

Looks like this fell of the wagon at some point. I've tried to merge main into this PR but I'm seeing more conflicts than I can solve.

Is it worth salvaging this PR?

@mpkorstanje, haven't evaluated the scope of changes as such just yet, though for sure would be great to retain the contributions and review if something to take forward.

I was able to merge and resolve the conflicts from main locally - majorly conflicts with removal of duplicate code (#205), formatting (#286), type hints (#283) and migration of bin/ to scripts/ (#290); though intend to repeat and look more thoroughly when have more time before pushing.

@ehuelsmann
Copy link
Contributor

@kieran-ryan see my comments above regarding salvaging this PR: I've added the knowledge I gained from reviewing the Perl and JS implementations.

@ehuelsmann
Copy link
Contributor

I've done some work locally to merge main into this branch. The tests are not working any further than that there are parse errors (although there don't seem to be any token errors anymore, the parse errors are reported in the ndjson output?!).

@ehuelsmann
Copy link
Contributor

could push the work I have so far, if that's helpful.

@ehuelsmann
Copy link
Contributor

@temyers @davidjgoss @kieran-ryan I've pushed updates with all tests succeeding now, including the recent "longest-prefix" fix for #400. Could you check out this implementation and see if you have any remarks/comments?

GherkinInMarkdown is not a new feature; it's only being implemented in Python...
* Comments are not supported in GherkinInMarkdown, so don't expect it as output
* Table rows can't be matched as comments since comments are not supported in Markdown mode
(Since comments are not supported in GherkinInMarkdown.)
@ehuelsmann ehuelsmann force-pushed the python-enable-markdown-acceptance-tests branch from fb17eb0 to 31551ed Compare May 28, 2025 17:58
@mpkorstanje mpkorstanje merged commit 8f2bc62 into cucumber:main Jun 30, 2025
6 checks passed
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.

5 participants