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

Support alternative docstrings format (```) #214

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

ciaranmcnulty
Copy link
Contributor

Closes #155

src/Behat/Gherkin/Lexer.php Outdated Show resolved Hide resolved
src/Behat/Gherkin/Lexer.php Outdated Show resolved Hide resolved
@ciaranmcnulty ciaranmcnulty requested a review from stof February 4, 2021 22:49
@ciaranmcnulty
Copy link
Contributor Author

I can't particularly see a test to add aside from adding some fixtures (which would be duplicating the cucumber test)

@stof
Copy link
Member

stof commented Feb 5, 2021

if the cucumber tests cover that already, that's already enough to cover it IMO

@ciaranmcnulty
Copy link
Contributor Author

ciaranmcnulty commented Feb 5, 2021

I definitely felt the need for more lower-level tests here. Perhaps the various Protected methods inside the Lexer could be promoted into objects and tested more thoroughly?

For instance the meat of scanPyStringOp could be tested fairly comprehensively

@stof
Copy link
Member

stof commented Feb 17, 2021

I don't think these methods should be promoted (I don't even think they should be protected btw). That would couple tests to the exact implementation of the parsing.
To me, this should be covered by feeding more valid and invalid inputs to the parser instead (which would then outlive any refactoring of the parsing implementation). And such inputs should ideally be contributed upstream rather than be kept in our repo (if they are not there already) so that official gherkin parsers also benefit from these tests (and we are sure that we have the same behavior).

@ciaranmcnulty
Copy link
Contributor Author

Cool. Contributing to the upstream tests is slightly harder than it could be, but the documentation has recently improved so I'll be adding some in

@ciaranmcnulty ciaranmcnulty merged commit a582f60 into Behat:master Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backtick docstrings not yet supported
2 participants