Skip to content
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

Implement the official gherkin parser #698

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

jsa34
Copy link

@jsa34 jsa34 commented Sep 5, 2024

Implementation of the official Gherkin parser.
Trying to maintain the public API and current codebase as much as possible. I was thinking little steps, and introduce new features/refactoring after we know it works like-for-like with current feature set.

Credit to @elchupanebrej for his implementation I referenced in pytest-bdd-ng

Breaking changes
Some of the previous features that are not Gherkin-compliant I had to remove. I have tried to find workarounds where possible.

Changes include:

  • multiline steps not using triple quotes no longer work
  • tags cannot have spaces in them

NB: This simply replaces the parsing logic with the Gherkin official parser, but the current parser does some additional logic and makes custom entities that are needed for test execution. I have coerced the Gherkin Document data to the existing model (minimising change), but I have raised a new MR (#699) that completely removes the existing parser to replace with Gherkin Document data generated. Not sure which is best route to take.

…n public API and current codebase as much as possible)
@jsa34 jsa34 changed the title First commit for using the official gherkin parser Implement the official gherkin parser Sep 5, 2024
@jsa34 jsa34 marked this pull request as ready for review September 5, 2024 17:42
Copy link

sourcery-ai bot commented Sep 5, 2024

🧙 Sourcery has finished reviewing your pull request!


Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

@jsa34
Copy link
Author

jsa34 commented Sep 6, 2024

@youtux please let me know if this is the right way to go.
I also see that in the parser, a few of the entities seem to reference each other, and I wonder if we might be able to avoid it by refactoring (e.g. Step has an optional Background, and Background has a list of steps) - this would be a future endeavour. I'm just thinking it's best to clean up the parser and entities before adding features if this is needed.

@jsa34
Copy link
Author

jsa34 commented Sep 6, 2024

Finished tinkering and fixing things for the CI jobs :)

@jsa34
Copy link
Author

jsa34 commented Sep 6, 2024

@sourcery-ai review

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

Copy link
Contributor

@youtux youtux left a comment

Choose a reason for hiding this comment

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

Good stuff! A few comments, but we are in the right track! Thank you for your effort

pyproject.toml Outdated Show resolved Hide resolved
src/pytest_bdd/gherkin_parser.py Outdated Show resolved Hide resolved
src/pytest_bdd/gherkin_parser.py Outdated Show resolved Hide resolved
tests/feature/test_outline.py Outdated Show resolved Hide resolved
tests/feature/test_steps.py Outdated Show resolved Hide resolved
tests/feature/test_steps.py Outdated Show resolved Hide resolved
@jsa34
Copy link
Author

jsa34 commented Sep 8, 2024

I'll take a look into your comments.

Most I can already see I agree with.

I was in 2 minds about Pydantic - it's made model validation much easier when ingesting the gherkin document dict, but I can absolutely see your point. I will have a look :)

@jsa34
Copy link
Author

jsa34 commented Sep 8, 2024

Thank you for your feedback and time to look at this, @youtux!

I've resolved most comments - just a couple of things:

  • given_when_then comments' response -> just need a steer
  • the escaping I have fixed the way I could best see, but would appreciate a second pair of eyes.

Copy link
Contributor

@youtux youtux left a comment

Choose a reason for hiding this comment

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

can you also add a changelog entry in CHANGES.rst with all the backwards incompatible changes?
So far I could see that we don't accept spaces in tags and that now Feature: is required in the feature file definitions.

tests/feature/test_wrong.py Outdated Show resolved Hide resolved
tests/feature/test_background.py Outdated Show resolved Hide resolved
src/pytest_bdd/gherkin_parser.py Show resolved Hide resolved
src/pytest_bdd/gherkin_parser.py Show resolved Hide resolved
tests/feature/test_outline.py Show resolved Hide resolved
tests/parser/test_parser.py Outdated Show resolved Hide resolved
@youtux
Copy link
Contributor

youtux commented Sep 14, 2024

I just tried parsing this feature:

Feature: Background support

    Scenario: Basic usage
        And foo

and it fails with an unhandled exception:

============================= test session starts ==============================
platform darwin -- Python 3.12.2, pytest-8.3.2, pluggy-1.5.0
rootdir: /private/var/folders/dy/_0849zr93fj1td2_07t4ybpr0000gn/T/pytest-of-youtux/pytest-5/test_background_basic0
plugins: bdd-7.2.0, xdist-3.6.1
collected 1 item

test_background_basic.py F                                               [100%]

=================================== FAILURES ===================================
_______________________________ test_background ________________________________

self = <[AttributeError("'Step' object has no attribute 'lines'") raised in repr()] Step object at 0x107bd4e30>

    def __str__(self) -> str:
        """Return a string representation of the step.
    
        Returns:
            str: A string representation of the step.
        """
>       return f'{self.type.capitalize()} "{self.name}"'
E       AttributeError: 'NoneType' object has no attribute 'capitalize'

/Users/youtux/Developer/pytest-bdd/src/pytest_bdd/parser.py:255: AttributeError
=========================== short test summary info ============================
FAILED test_background_basic.py::test_background - AttributeError: 'NoneType'...
============================== 1 failed in 0.07s ===============================

can you fix this and maybe add a test for it?

@youtux
Copy link
Contributor

youtux commented Sep 14, 2024

by the way, that was caught running mypy, you might want to run that too to find possible other errors

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 93.79310% with 27 lines in your changes missing coverage. Please review.

Project coverage is 94.59%. Comparing base (d517a80) to head (94bf691).

Files with missing lines Patch % Lines
tests/parser/refactor_parser.py 0.00% 11 Missing ⚠️
src/pytest_bdd/gherkin_parser.py 96.91% 3 Missing and 2 partials ⚠️
src/pytest_bdd/parser.py 95.65% 2 Missing and 3 partials ⚠️
src/pytest_bdd/scenario.py 75.00% 1 Missing and 1 partial ⚠️
src/pytest_bdd/utils.py 0.00% 2 Missing ⚠️
src/pytest_bdd/generation.py 66.66% 0 Missing and 1 partial ⚠️
src/pytest_bdd/steps.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
- Coverage   95.35%   94.59%   -0.76%     
==========================================
  Files          50       54       +4     
  Lines        1828     1999     +171     
  Branches      201      232      +31     
==========================================
+ Hits         1743     1891     +148     
- Misses         53       71      +18     
- Partials       32       37       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

They are not really failures, just deprecation warnings that we treat as errors.
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.

2 participants