[Bug] bad-indent false positive if [Documentation] used in test case that uses Test Template

[sekwahs]

What happened?

Seeing "Line is under-indented (bad-indent)" when a test case for which Test Template is used contains a [Documentation] line.

Example:

*** Settings ***

Test Template  Example

*** Test Cases ***

Example  argument=abc
  [Documentation]  Documentation text

*** Keywords ***

Example
  [Arguments]  ${argument}
  Log  ${argument}

What command/code did you try to run?

robocop example.robot

What is the full error message?

example.robot:8:1 [W] 1008 Line is under-indented (bad-indent)

What did you expect to happen instead?

Did not expect this to trigger a warning.

Operating System

Linux

Robocop version

3.0.0

[mnojek]

Hey @sekwahs and thanks for the report.
Unfortunately, I can't reproduce it, because you pasted the code in an unformatted way, so all the whitespaces were removed, and that is crucial to understand the issue.

Can you please try to enclose your code inside a triple ``` character, so that the code saves the original formatting?

```
Like this
```

will result in:

Like this

[sekwahs]

Updated example.

…

On Wed, Mar 29, 2023, 9:08 AM Mateusz Nojek ***@***.***> wrote: Hey @sekwahs <https://github.com/sekwahs> and thanks for the report. Unfortunately, I can't reproduce it, because you pasted the code in an unformatted way, so all the whitespaces were removed, and that is crucial to understand the issue. Can you please try to enclose your code inside a triple ``` character, so that the code saves the original formatting? ``` Like this ``` will result in: Like this — Reply to this email directly, view it on GitHub <#808 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A23OLD5RXCOIGFUYEQWNHWLW6Q64JANCNFSM6AAAAAAWL745YE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mnojek]

OK, after reviewing this case, I started thinking if this rule should be applied to templated suites at all.

The official documentation is very scarce about using test case settings along with the templated suite. Actually, it does not mention it at all, and I see people doing it (because it's allowed). There are no examples how to do it, so the users come up with different approaches.

The code you shared with us looks correct (apart from the other formatting, which violates many Robocop rules, but this is not our concern here) and is properly parsed by Robot Framework.

Honestly, I don't know what should be the approach for formatting templated suites.

In our test data we have such example (that reports no Robocop issues):

*** Test Cases ***                  USERNAME            PASSWORD
Invalid User Name                   [Tags]              foo
                                    invalid             ${VALID PASSWORD}
Invalid Password                    [Documentation]     foo
                                    [Tags]              bar
                                    ${VALID USER}       invalid
Invalid User Name and Password      [Tags]              baz
                                    invalid             invalid
Empty User Name
                                    ${EMPTY}            ${VALID PASSWORD}
Empty Password                      ${VALID USER}       ${EMPTY}
                                    [Tags]              spam  eggs
Empty User Name and Password        ${EMPTY}            ${EMPTY}

and also yours above, which represents different approach.

BTW, your code would not report any issues if written like this:

Example  argument=abc
         [Documentation]  Documentation text

I need to ask for an advice here @bhirsz and maybe @pekkaklarck.
What is the intended way to indent templated test cases when the users want to also use test case settings together with them?

[mnojek]

Ok, I gave a lot of time to think about it and I must say, that this behavior is not a bug.

Robocop purpose is to serve as a tool that checks whether the code aligns with good practices or contains errors.
There are 3 levels of severity: errors, warnings and infos. Errors are something that can break the code and work in an unintended way. Warnings let you know that you probably haven't used common or standard way of writing the code. Infos are there just to give some hints or suggestions, but basically the code is fine.

In your code snippet, Robocop complained about the line being under-indented and... it's true.
Your code does not follow common practice. It works, so the report is not an error, but it's a warning saying that there are probably better ways to format your code.

As suggested in my previous comment - if you align it like this:

Example  argument=abc
         [Documentation]  Documentation text

Robocop will not complain.

If you don't want to receive this message, you can either disable it from the command line (robocop -e 1008) completely, or just make an exception in this line adding a comment with a disabler:

Example  argument=abc
  [Documentation]  Documentation text  # robocop: disable=1008

@sekwahs Feel free to share what you feel about it, but this will not be fixed, since it's not a bug. It's just how Robocop works.

[sekwahs]

We differ on our interpretation of the word indentation. In my programming experience indentation is the amount of whitespace at the beginning of a line and is typically used to indicate logical syntactic levels within code; spacing between language elements is not considered to be indentation.

In this new 3.0.0 implementation of bad-indent the meaning of the word indentation is apparently alignment of words across vertical sections of code regardless of their actual relationship to each other. In the cited example, "argument=abc" has no inherent or implied relationship to the line that starts with "[Documentation]" and thus should require no relative alignment across lines.

Perhaps my confusion is based on the name of the rule, bad-indent, which implies it deals solely with indentation. I see now that the documentation for the rule says "Line is misaligned or indent is invalid," so the rule actually looks for more than indentation. In this case I would argue that alignment checking should be covered by a different rule than indentation checking since a) the rule is bad-indent, not bad-alignment, and b) indentation is inherently important in robot and consistency promotes readability, but alignment of unrelated language elements across lines is not inherently important, is not restricted by robot, and whether or how such alignment should be done is a very debatable topic and a choice which should be left to programmers. I could argue at length on why the alignment suggested above between the start of a line within a keyword body should have no connection with the alignment of arguments to the keyword, but that isn't the important issue here; the difference between indentation checking and alignment checking is what is relevant here, along with the ability to separate the two types of checking when enforcing rules.

If with release 3.0.0 there are new or old rules that allow checking only indentation without checking alignment based on language elements following the initial indentation, please point me to these so I can attempt to find a solution that provides the functionality desired by my development team.

[bhirsz]

I agree with the notion that "bad-indent" should mark problems with the indent, not the alignment. In this case we are actually iterating over statements in the node and we're a not checking if the line is the same.

In case of:

Example  argument=abc
  [Documentation]  Documentation text  # robocop: disable=1008

We're getting following statements:

• Example
• argument=abc
• [Documentation] Documentation text # robocop: disable=1008

And then we're checking for indentation for each statement.

I think we should:

• if the indent is the same as the previous statement, we should report new rule "bad-alignment"
• otherwise, check the indent and optionally report "bad-indent"

We could also wait for Robot Framework style guide decision on indentation etc, they have this dicussion on their roadmap. Unfortunately "bad-indent" rule is one of most unstable rules (we keep changing its implementation) and it could be good to do it once and for good.

[bhirsz]

And the last statement is the reason we think it would be best to "park" this issue for now. We don't want to keep changing the rule and it's best to wait until there is official guideline in the Robot Framework (and it's in the progress). There is very high chance that we will follow approach proposed by you in the end but still, we don't want to rush it to avoid changing this rule yet again.

In the meantime you can exclude bad-indent and bad-block-indent and create your own custom rule. Below there is version that should work like you want:

class IndentChecker(VisitorChecker):
    """Checker for indentation violations."""

    reports = ("bad-indent-rule", "bad-block-indent-rule")  # just use any name different than official bad-indent one

    def __init__(self):
        self.indents = []
        self.parent_indent = 0
        # used to ignore indents from statements in the same line as parent, i.e. Inline IFs
        self.parent_line = 0
        self.last_statement_line = 0
        # used to denote end of keyword/test for comments indents
        self.end_of_node = False
        
        super().__init__()

    def visit_File(self, node):  # noqa
        self.indents = []
        self.parent_indent = 0
        self.parent_line = 0
        self.last_statement_line = 0
        self.end_of_node = False
        self.generic_visit(node)

    def visit_TestCase(self, node):  # noqa
        end_index = index_of_first_standalone_comment(node)
        with block_indent(self, node):
            for index, child in enumerate(node.body):
                if index == end_index:
                    self.end_of_node = True
                self.visit(child)

    visit_Keyword = visit_TestCase  # noqa

    def visit_TestCaseSection(self, node):  # noqa
        self.check_standalone_comments_indent(node)

    def visit_KeywordSection(self, node):  # noqa
        self.check_standalone_comments_indent(node)

    def check_standalone_comments_indent(self, node):
        # comments before first test case / keyword
        for child in node.body:
            if (
                getattr(child, "type", "") == Token.COMMENT
                and getattr(child, "tokens", None)
                and child.tokens[0].type == Token.SEPARATOR
            ):
                self.report(
                    "bad-indent",
                    bad_indent_msg="Line is over-indented",
                    node=child,
                    col=1,
                    end_col=token_col(child, Token.COMMENT),
                )
        self.generic_visit(node)

    def visit_For(self, node):
        self.visit_Statement(node.header)
        with block_indent(self, node):
            for child in node.body:
                self.visit(child)
        self.visit_Statement(node.end)

    visit_While = visit_ForLoop = visit_For

    def get_common_if_indent(self, node):
        indents = count_indents(node)
        head = node
        while head.orelse:
            head = head.orelse
            indents += count_indents(head)
        most_common = most_common_indent(indents)
        self.indents.append(most_common)

    def get_common_try_indent(self, node):
        indents = count_indents(node)
        head = node
        while head.next:
            head = head.next
            indents += count_indents(head)
        most_common = most_common_indent(indents)
        self.indents.append(most_common)

    def visit_statements_in_branch(self, node):
        with replace_parent_indent(self, node):
            for child in node.body:
                self.visit(child)

    def visit_If(self, node):
        self.visit_Statement(node.header)
        if node.type == "INLINE IF":
            return
        self.get_common_if_indent(node)
        self.visit_statements_in_branch(node)
        if node.orelse is not None:
            self.visit_IfBranch(node.orelse)
        self.indents.pop()
        self.visit_Statement(node.end)

    def visit_IfBranch(self, node):  # noqa
        indent = self.indents.pop()
        self.visit_Statement(node.header)
        self.indents.append(indent)
        self.visit_statements_in_branch(node)
        if node.orelse is not None:
            self.visit_IfBranch(node.orelse)

    def visit_Try(self, node):
        self.visit_Statement(node.header)
        self.get_common_try_indent(node)
        self.visit_statements_in_branch(node)
        if node.next is not None:
            self.visit_TryBranch(node.next)
        self.indents.pop()
        self.visit_Statement(node.end)

    def visit_TryBranch(self, node):  # noqa
        indent = self.indents.pop()
        self.visit_Statement(node.header)
        self.indents.append(indent)
        self.visit_statements_in_branch(node)
        if node.next is not None:
            self.visit_TryBranch(node.next)

    def get_required_indent(self, statement):
        if isinstance(statement, Comment) and self.end_of_node:
            return 0
        if self.param("bad-indent", "indent") != -1:
            return self.param("bad-indent", "indent") * len(self.indents)
        return self.indents[-1]

    def visit_Statement(self, statement):  # noqa
        if statement is None or isinstance(statement, EmptyLine) or not self.indents:
            self.last_statement_line = statement.lineno
            return
        # Ignore indent if current line is on the same line as parent, i.e. test case header or inline IFs
        if self.parent_line == statement.lineno:
            self.last_statement_line = statement.lineno
            return
         if self.last_statement_line == statement.lineno:    # ignore bad alignment
            return
        self.last_statement_line = statement.lineno
        indent = get_indent(statement)
        if self.parent_indent and (indent - 2 < self.parent_indent):
            self.report(
                "bad-block-indent",
                node=statement,
                col=1,
                end_col=indent + 1,
            )
            return
        req_indent = self.get_required_indent(statement)
        if indent == req_indent:
            return
        over_or_under = "over" if indent > req_indent else "under"
        self.report(
            "bad-indent",
            bad_indent_msg=f"Line is {over_or_under}-indented",
            node=statement,
            col=1,
            end_col=indent + 1,
        )

The code could be a lot shorter if you just want to check strict indentation, not "most common indent in block" which is also allowed.