Python: Handle empty lines in Description AST node and add test cases when there is only a comment between the scenario line and the first step

[gasparnagy]

🤔 What's changed?

Added two more test cases according to #373

⚡️ What's your motivation?

Better understand #373 and see impact of #334

🏷️ What kind of change is this?

• 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

📋 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.
• 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]

Looks like the AST builder is making assumptions it shouldn't:

self.current_node.add(node.rule_type, self.transform_node(node))
  File "/home/runner/work/gherkin/gherkin/python/gherkin/ast_builder.py", line 272, in transform_node
    last_non_empty = next(
StopIteration

@jsa34 @youtux or anyone else with Python knowledge would you be able to dig into this?

[jsa34]

I'll check out the code and debug :)

[jsa34]

So, it looks like for v30.0.4 Gherkin, the AST builder didn't consider that there was a "Description" node when there is just a comment - it looks like it was discarded and so the "Description" was non-existent in this circumstance.
Since v31.0.0, the comment makes it think there is a Description AST node (wrongly or rightly - I'm not sure what is intended behaviour in the test scenarios above (i.e. description only with comment(s)).

My naive assessment just looking at the Python logic would be:

1. If we expect that the AST builder can have a Description node with an empty list of line_tokens and this is valid, that we need to change the code to handle this - something along the lines of:

    line_tokens = node.get_tokens("Other")

    if not line_tokens:  # Handle the case where there are no tokens
        return ""

    # Trim trailing empty lines
    last_non_empty = next(
        (i for i, j in reversed(list(enumerate(line_tokens))) if j.matched_text),
        -1,  # Default value if no non-empty tokens are found
    )

    description = "\n".join(
        [token.matched_text for token in line_tokens[: last_non_empty + 1]]
    )

    return description

2. If we should only have a Description node if there are actually line_tokens associated with it, this looks like a regression, as that was how the old logic seemed to work, and I'm not sure how to best resolve this.

I hope this is helpful!

[mpkorstanje]

It would be advisable to use the other implementations as a reference.

gherkin/python/gherkin/ast_builder.py

Lines 269 to 279 in 1371351

	elif node.rule_type == "Description":
	line_tokens = node.get_tokens("Other")
	# Trim trailing empty lines
	last_non_empty = next(
	i for i, j in reversed(list(enumerate(line_tokens))) if j.matched_text
	)
	description = "\n".join(
	[token.matched_text for token in line_tokens[: last_non_empty + 1]]
	)
	return description

gherkin/javascript/src/AstBuilder.ts

Lines 217 to 227 in 1371351

	case RuleType.Description: {
	let lineTokens = node.getTokens(TokenType.Other)
	// Trim trailing empty lines
	let end = lineTokens.length
	while (end > 0 && lineTokens[end - 1].line.trimmedLineText === '') {
	end--
	}
	lineTokens = lineTokens.slice(0, end)
	return lineTokens.map((token) => token.matchedText).join('\n')
	}

gherkin/java/src/main/java/io/cucumber/gherkin/GherkinDocumentBuilder.java

Lines 162 to 174 in 1371351

	case Description: {
	List<Token> lineTokens = node.getTokens(TokenType.Other);
	// Trim trailing empty lines
	int end = lineTokens.size();
	while (end > 0 && lineTokens.get(end - 1).matchedText.matches("\\s*")) {
	end--;
	}
	lineTokens = lineTokens.subList(0, end);
	return lineTokens.stream()
	.map(t -> t.matchedText)
	.collect(Collectors.joining("\n"));
	}

Replacing the list comprehension with a simple while loop should solve the problems and keep the implementations aligned.

[jsa34]

Apologies - I didn't look at other language implementations as I just focussed on debugging the behaviour I can see in python exclusively and wasn't sure what the intended behaviour was.

A while loop would work great and thinking about this from what I would write from a pythonic implementation:

line_tokens = node.get_tokens("Other")
tokens = list(line_tokens)
# Trim trailing empty lines 
while tokens and not tokens[-1].matched_text:
    tokens.pop()
return "\n".join(token.matched_text for token in tokens)

I checked locally and this fixes the issue.

[mpkorstanje]

So Gaspar made a branch directly in this repository. As a member of the Cucumber org you can push directly to that branch.

[jsa34]

I updated the PR title to reflect that this now has the fix for the issue these tests exposed, too.
Thanks @gasparnagy for your help and thanks @mpkorstanje for your patience and support!