[Core] Fix PrettyFormatter exception on nested arguments #1200

mlvandijk · 2017-08-17T18:07:45Z

Summary

Fixed to ignore formatting for nested capture groups (arguments), as they are already formatted as an argument as part of the enclosing capture group (argument), thus no longer throwing StringIndexOutOfBoundsException which prevented additional scenarios/ features from running (#619).

Details

Added 2 tests to verify formatting of nested capture groups
Added 2 checks on variables passed to substring() to prevent StringIndexOutOfBoundsException (see more below).
Also did a minor refactor (=renamed these variables) for clarity.
Additionally, fixed a typo in PrettyFormatterTest

Motivation and Context

Nested capture groups could not be handled by the PrettyFormatter. Since each capture groups maps to an argument, this sort of gives an argument in an argument. A nested argument starts before the enclosing argument ends, throwing a StringIndexOutOfBoundsException on stepText.substring().

public String substring(int beginIndex, int endIndex) throws: IndexOutOfBoundsException - if the beginIndex is negative, or endIndex is larger than the length of this String object, or beginIndex is larger than endIndex.

beginIndex = argument offset + length of argument
endIndex = argumentOffset

The expected value for argument.offset is always negative for nested capture groups, as a nested argument starts before the previous (=enclosing) argument ends.
This means:

beginIndex could be < 0, throwing an Exception -> Added check to verify beginIndex >= 0
beginIndex could be larger than endIndex -> Added check verify endIndex < beginIndex
** Note: endIndex (argumentOffset) will not be larger than the length of this String object (stepText) -> no check needed

How Has This Been Tested?

Reproduce original bug by creating a test expecting StringIndexOutOfBoundsException. Removed this test once it failed after implementation of the fix.
Added 2 tests to test that nested argument(s) is/are formatted as part of the enclosing argument (and no Exception is thrown).
Clean/install cucumber-jvm core (i.e. checking all other tests still pass).

Screenshots (if appropriate):

N/A

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly.

If necessary, add some documentation about PrettyFormatter (provided by @mpkorstanje ):

The pretty formatter creates a pretty looking text on the command line output. On windows this is white and plain, but when using a Unix Ansi compatible terminal this output is colored and has formatting.

Color and other formatting is enabled by printing a special escape sequence at the start of the part that should be colored. At the end of the markup a reset symbol is printed.

So the text Given text arg1 text arg2 is rendered as "Given text arg1 text arg2".

You can see an example of this in should_mark_arguments_in_steps.

A nested capture group is a capture group inside a regex that is placed inside another capture group. For example: the order is placed( and (not yet )? confirmed)?

This means that all these are valid steps:

Given the order is placed
Given the order is placed and confirmed
Given the order is placed and not yet confirmed

The step Given the order is placed and not yet confirmed has two Arguments.

new Argument(20, "and not yet confirmed")
new Argument(-21, "not yet")

And should be formatted as: "Given the order is placed and not yet confirmed". It is acceptable to completely skip the nested argument as the enclosing step has already been formatted from end to end.

mlvandijk · 2017-08-17T19:01:21Z

Squashed all commits to this branch into one commit.
Additional commit to resolve merge conflict with master.

brasmusson · 2017-08-17T19:07:46Z

I recommend you to rebase pull request branches instead of merging the master branch to a pull-request branch. My experience is that the latter confuses Github so it does not properly show only the changes of the pull request File Changed tab.

mlvandijk · 2017-08-18T13:09:43Z

Reverted accidental merge to master
Rebased master to (under?) this branch.

mpkorstanje

Empty. See next.

mpkorstanje · 2017-08-20T09:42:43Z

core/src/main/java/cucumber/runtime/formatter/PrettyFormatter.java

            }
        }
-        if (textStart != stepText.length()) {
-            String text = stepText.substring(textStart, stepText.length());
+        if (beginIndex != stepText.length() && beginIndex >= 0) {


beginIndex >= 0 is not required here. We skip all nested capture groups (and all nested capture groups have a positive index).

Huh, you're right! I added it before, because it was throwing exception here also.. will update

mpkorstanje · 2017-08-20T09:48:30Z

core/src/main/java/cucumber/runtime/formatter/PrettyFormatter.java

        StringBuilder result = new StringBuilder(textFormat.text(keyword));
        for (Argument argument : arguments) {
            // can be null if the argument is missing.
            if (argument.getOffset() != null) {
-                String text = stepText.substring(textStart, argument.getOffset());
+                int argumentOffset = argument.getOffset();
+                if (argumentOffset < beginIndex ) { // a nested argument starts before the enclosing argument ends; ignore it when formatting


It is okay to place line comments above the block or line they reference. Especially if the comment is long.

mpkorstanje · 2017-08-20T09:50:59Z

core/src/test/java/cucumber/runtime/formatter/PrettyFormatterTest.java

+        Formats formats = new AnsiFormats();
+        Argument enclosingArg = new Argument(20, "and not yet confirmed");
+        Argument nestedArg = new Argument(-17, "not yet ");
+        Argument nestedNestedArg = new Argument(-5, "yet ");


I had forgotten to check if the examples were correct by writing an integration test. I have done so now.

In JdkPatternArgumentMatcherTest you can add this test to reproduce the examples:

@Test public void canHandleNestedCaptureGroups() throws UnsupportedEncodingException { assertVariables("the order is placed( and( not yet)? confirmed)?", "the order is placed and not yet confirmed", " and not yet confirmed", 19, " not yet", 23); }

Important to note is that the indexes are not negative. They just come before the end of the previous capture group.

will do. Also, this answers my question of where to verify the regex actually does get parsed correctly etc... :)

You've added the test but you didn't update the index values in the PrettyFormatterTest.

mpkorstanje · 2017-08-20T09:57:13Z

core/src/main/java/cucumber/runtime/formatter/PrettyFormatter.java

                result.append(textFormat.text(text));
            }
            // val can be null if the argument isn't there, for example @And("(it )?has something")
            if (argument.getVal() != null) {
                result.append(argFormat.text(argument.getVal()));
-                textStart = argument.getOffset() + argument.getVal().length();
+                int argumentEnd = argument.getOffset() + argument.getVal().length(); // end of current argument; can be < 0 for nested argument
+                beginIndex = argumentEnd;


It is okay to combine both lines into a single statement.

Yes, I know (it was before). I added extra step to make it clearer (at least to me) what is going on. will change it back.

coveralls · 2017-08-20T13:36:49Z

Coverage increased (+0.01%) to 56.198% when pulling db9a533 on mlvandijk:pretty-formatter-nested-args into 161e765 on cucumber:master.

coveralls · 2017-08-20T14:08:53Z

Coverage increased (+0.01%) to 56.198% when pulling 5b65317 on mlvandijk:pretty-formatter-nested-args into 161e765 on cucumber:master.

coveralls · 2017-08-20T22:25:21Z

Coverage increased (+0.01%) to 56.198% when pulling d1e6c23 on mlvandijk:pretty-formatter-nested-args into 161e765 on cucumber:master.

coveralls · 2017-08-20T22:26:18Z

Coverage increased (+0.01%) to 56.198% when pulling d1e6c23 on mlvandijk:pretty-formatter-nested-args into 161e765 on cucumber:master.

lock · 2018-10-24T23:13:44Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mlvandijk requested a review from mpkorstanje August 17, 2017 18:09

mlvandijk mentioned this pull request Aug 19, 2017

Fix PrettyFormatter Exception for nested capture groups #1197

Merged

6 tasks

mpkorstanje reviewed Aug 20, 2017

View reviewed changes

mpkorstanje requested changes Aug 20, 2017

View reviewed changes

Fix PrettyFormatter exception on nested arguments

5b65317

mlvandijk added 2 commits August 20, 2017 21:41

Fix PrettyFormatter nested arguments

70c6c37

Fix PrettyFormatter nested arguments

d1e6c23

mpkorstanje approved these changes Aug 20, 2017

View reviewed changes

mpkorstanje merged commit 0adf669 into cucumber:master Aug 20, 2017

mlvandijk deleted the pretty-formatter-nested-args branch August 21, 2017 12:39

mlvandijk mentioned this pull request Aug 21, 2017

[JVM] Pretty Formatter error #619

Closed

mlvandijk mentioned this pull request May 24, 2018

gherkin: StringIndexOutOfBoundsException in step cucumber/common#396

Closed

lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Fix PrettyFormatter exception on nested arguments #1200

[Core] Fix PrettyFormatter exception on nested arguments #1200

mlvandijk commented Aug 17, 2017

mlvandijk commented Aug 17, 2017

brasmusson commented Aug 17, 2017

mlvandijk commented Aug 18, 2017

mpkorstanje left a comment •

edited

Loading

mpkorstanje Aug 20, 2017

mlvandijk Aug 20, 2017

mpkorstanje Aug 20, 2017

mlvandijk Aug 20, 2017

mpkorstanje Aug 20, 2017

mlvandijk Aug 20, 2017 •

edited

Loading

mpkorstanje Aug 20, 2017

mpkorstanje Aug 20, 2017

mlvandijk Aug 20, 2017 •

edited

Loading

coveralls commented Aug 20, 2017

coveralls commented Aug 20, 2017

coveralls commented Aug 20, 2017

coveralls commented Aug 20, 2017

lock bot commented Oct 24, 2018

[Core] Fix PrettyFormatter exception on nested arguments #1200

[Core] Fix PrettyFormatter exception on nested arguments #1200

Conversation

mlvandijk commented Aug 17, 2017

Summary

Details

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mlvandijk commented Aug 17, 2017

brasmusson commented Aug 17, 2017

mlvandijk commented Aug 18, 2017

mpkorstanje left a comment • edited Loading

Choose a reason for hiding this comment

mpkorstanje Aug 20, 2017

Choose a reason for hiding this comment

mlvandijk Aug 20, 2017

Choose a reason for hiding this comment

mpkorstanje Aug 20, 2017

Choose a reason for hiding this comment

mlvandijk Aug 20, 2017

Choose a reason for hiding this comment

mpkorstanje Aug 20, 2017

Choose a reason for hiding this comment

mlvandijk Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

mpkorstanje Aug 20, 2017

Choose a reason for hiding this comment

mpkorstanje Aug 20, 2017

Choose a reason for hiding this comment

mlvandijk Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Aug 20, 2017

coveralls commented Aug 20, 2017

coveralls commented Aug 20, 2017

coveralls commented Aug 20, 2017

lock bot commented Oct 24, 2018

mpkorstanje left a comment •

edited

Loading

mlvandijk Aug 20, 2017 •

edited

Loading

mlvandijk Aug 20, 2017 •

edited

Loading