Python: Make tests pass against shared test data

[aslakhellesoy]

In #9 we introduced shared test data. The Python implementation should be updated to:

• Run all tests in testdata/*.yml
• Update the implementations to make those tests pass

[jenisys]

Pull #95 contains an implementation to use the test data for Python.

• Run all tests in testdata/*.yml
• Update the implementations to make those tests pass

HINTS:

• The backslash-escaping mechanism is not implemented yet for Python
• Therefore, tests that use the backslash-escaping mechanism are currently skipped

[luke-hill]

@jenisys - Not sure if you're still around / maintaining and checking python. But 2/3 of the shared tests are now ran. Just the errors ones aren't running. This feels like this issue is close to being "close off-able"

cc/ @brasmusson

[kieran-ryan]

...2/3 of the shared tests are now ran. Just the errors ones aren't running. This feels like this issue is close to being "close off-able"

@luke-hill, interested in your thoughts based on diff below. The Python error messages have higher granularity; so we may want to consolidate a similar approach into the other parsers; and/or limit the expected message to a suitable substring and check matches across implementations.

Some error messages improvements in recent Python releases which may be of relevance regards enhancements.

Python error messages diff

@a @b or

- Tag expression "@a @b or" could not be parsed because of syntax error: Expected operator.
+ Syntax error. Expected operator after @a
+ Expression: @a @b or
+ _______________^ (HERE)

@a and (@b not)

- Tag expression "@a and (@b not)" could not be parsed because of syntax error: Expected operator.
+ Syntax error. Expected operator after @b
+ Expression: @a and ( @b not )
+ ________________________^ (HERE)

@a and (@b @c) or

- Tag expression "@a and (@b @c) or" could not be parsed because of syntax error: Expected operator.
+ Syntax error. Expected operator after @b
+ Expression: @a and ( @b @c ) or
+ ________________________^ (HERE)

@a and or

- @a and or" could not be parsed because of syntax error: Expected operand.
+ Syntax error. Expected operand after and
+ Expression: @a and or
+ ___________________^ (HERE)

or or

- Tag expression "or or" could not be parsed because of syntax error: Expected operand.
+ Syntax error. Expected operand after BEGIN
+ Expression: or or
+ _____________^ (HERE)

a and or

- Tag expression "a and or" could not be parsed because of syntax error: Expected operand.
+ Syntax error. Expected operand after and
+ Expression: a and or
+ __________________^ (HERE)

a b

- Tag expression "a b" could not be parsed because of syntax error: Expected operator.
+ Syntax error. Expected operator after a
+ Expression: a b
+ ______________^ (HERE)

( a and b ) )

- Tag expression "( a and b ) )" could not be parsed because of syntax error: Unmatched ).
+ Missing '(': Too few open-parens in: ( a and b ) )
+ Expression: ( a and b ) )
+ ________________________^ (HERE)

x\ or y

- Tag expression "x\\ or y" could not be parsed because of syntax error: Expected operator.
+ Syntax error. Expected operator after x or
+ Expression: x or y
+ _________________^ (HERE)

( ( a and b )

- Tag expression "( ( a and b )" could not be parsed because of syntax error: Unmatched (.
+ Unclosed '(': Too many open-parens in: ( ( a and b )

[luke-hill]

This should come from a consensus at one of the contributor meetings.

Ideally the shared ones should conform. I think the issue here is development came before testing, which ideally shouldn't happen. But such is the way with real life.

P.S. Looking at some of the above. I think there are some errors in the messages.

or or => The ^ marker is 1 char too far right
( a and b ) ) => The previous error probably was more informative than this one now.
x\ or y => seems to be sanitizing out the \ char
( ( a and b ) same as the opposite one.

[kieran-ryan]

Cheers @luke-hill! 🙌 I might take a look at correcting the error messages as you've mentioned if I get chance - was interested to look at enhancing and maybe taking a similar format to the latest Python error messages style - good to have direction

[mpkorstanje]

I'm definitely in favor of friendlier messages! Cucumber expressions currently uses the format below. Which contains the same elements as the Python error messages but with a slightly different formatting.

https://github.com/cucumber/cucumber-expressions/blob/212dc4d30f58ff7a3bcc226c54f68c8b3b03769d/testdata/cucumber-expression/matching/does-not-allow-alternation-with-empty-alternative-by-adjacent-optional.yaml#L10

expression: three (brown)/black mice
text: three brown mice
exception: |-
  This Cucumber Expression has a problem at column 7:

  three (brown)/black mice
        ^-----^
  An alternative may not exclusively contain optionals.
  If you did not mean to use an optional you can use '\(' to escape the '('

But it seems prudent to me to make sure the existing tests work before we start making improvements.

@kieran-ryan I don't quite understand your diff. I don't see any code in the current Python implementation that would produce the error messages you are showing. Is this a proposal for improvement or how it currently works? If the latter, could you point out to me where in the code this is done?

[kieran-ryan]

@kieran-ryan I don't quite understand your diff. I don't see any code in the current Python implementation that would produce the error messages you are showing. Is this a proposal for improvement or how it currently works? If the latter, could you point out to me where in the code this is done?

@mpkorstanje, above diff would be how it currently works rather than a proposal. I've enabled the error message tests to run against the Python implementation - to show same, though the pytest output is not most the most user-friendly to read; may find useful in any case.

The code handling the error message is as below. Based on internal usage within the library, you can ignore the first conditional (error_index > len(parts)) - as believe is never hit; and for awareness, the second conditional is always hit (if message).

tag-expressions/python/cucumber_tag_expressions/parser.py

Lines 324 to 333 in 0ddbd2f

	def _make_error_description(message, parts, error_index):
	if error_index > len(parts):
	error_index = len(parts) # noqa
	good_text_size = len(" ".join(parts[:error_index]))
	error_pos = len("Expression: ") + good_text_size + 1
	template = "Expression: {expression}\n%s^ (HERE)" % ("_" * error_pos)
	if message: # noqa
	template = "{message}\n" + template
	expression = " ".join(parts)
	return template.format(message=message, expression=expression)

[kieran-ryan]

@mpkorstanje, @luke-hill I aligned the Python error messages on the attached pull request; with tests passing. Pull request 'changes' will give a good idea of the difference - though bear in mind that most tests are checking for a substring, so although appears information has been dropped in some cases, other parts of the error message should majorly retain that information - such as the ____^ (HERE) aspect. Interested in your perspective whether we should proceed with these error messages; or refactor any/all messages (particularly if consider there to be any regression).

[luke-hill]

I'm definitely "more" pro than con. So from me it's a 👍 but I would prefer that we "didn't" do this for python originally and proposed it for all flavours.

But I think that because python is a bit like Reqnroll where it's got a lot of custom components from pytest-bdd e.t.c. and it's not fully using a cucumber official implementation, we're likely to hit a few of these.

I think the tag expressions library got a lot of it's juice early in the inception stage/s, so giving them a facelift and an update I see only as a positive.

TL;DR - I like the updates, but would prefer any changes are fully unanimous across all flavours of tag-expressions