Initially empty test fails even if pre-run modifier adds content to it

[rikerfi]

It seems that adding a keyword to an empty test case fails when using prerunmodifier. Below example was working with RF 6.0.2, but not any more with 6.1.1. If test case has some keywords in it, then the modifier works fine.

.robot file

*** Test Cases ***
Pass with Log
    # Modifier adds keyword to this test case
    Log    Hello from .robot

Pass with Modifier
    # Modifier fails to add keyword
    # Log    Hello from .robot

k.py file

from robot.api import SuiteVisitor
from robot.running.model import Keyword


class K(SuiteVisitor):
    def start_suite(self, suite):
        for test in suite.tests:
            kw = Keyword('Log', args=['Hello from MODIFIER'])
            test.body.append(kw)

    def start_keyword(self, keyword: Keyword) -> bool | None:
        print(f'KEYWORD: {keyword.parent.name}:{keyword.name}')

run robot --pythonpath . --prerunmodifier k.K .

[pekkaklarck]

This is due to RF 6.1 detecting test being empty already at the parsing time (#4210) and that error being then reported during execution without checking is the test empty anymore. A workaround for the problem is clearing out the error explicitly by adding test.error = None to your modifier code. To play it safe, you could do that only if the test is initially empty.

It would be better if Robot wouldn't report this error if the test isn't empty anymore when it's run. It's hard to do that only with this particular parsing error, though, because the code for converting parsing errors to execution time errors is generic. We could look at what other parsing errors are reported with tests and see do we need to report them like this at all, or could the same conditions looked separately at the execution time. For example, executing an empty test will fail even if the test would be created programmatically.

[rikerfi]

The workaround is working well. Thanks, Pekka.

Do you @pekkaklarck think we could add a command line or other config that alters the behavior of an empty test case? The default should be Fail, but e.g. something like --empty Pass or --empty Skip could be provided.

Just my 2 cents.

[pekkaklarck]

I don't think making the overall behavior configurable is worth the effort. Most importantly, I don't see why an empty test ever should get some other state than FAIL. The problem here is that the test isn't empty anymore and thus there shouldn't be any error either.

[pekkaklarck]

I decided to fix this so that parsing errors with tests aren't reported further at all. We currently only validate that test body isn't empty and that's anyway validated again at the execution time. This is easy, but doesn't really work if there are parsing errors that we actually want to report further. We could, for example, report invalid settings like that that or there could be some new error situation. Anyway, we don't currently do anything like that so this ought to be fine.

I'll do the same change also with user keywords. They aren't typically modified by pre-run modifiers, they only can access keywords from the suite file and even that's pretty awkward via test.parent.resource.keywords, but keywords will likely be available for listeners in the future.

I don't think making the change with other structures like IF makes sense. There's a use case for having empty tests or keywords as templates, but having an empty IF or other control structure sounds really strange. If someone needs something strange like that, they can clear the error attribute as well.