robotframework-robocop icon indicating copy to clipboard operation
robotframework-robocop copied to clipboard

[Enhancement] Create a new rule to issue an INFO message when Test Setup and [Setup] or Test Teardown and [Teardown] are used in the same test suite

Open sparkymartin opened this issue 2 years ago • 9 comments

When Test Setup and [Setup] or Test Teardown and [Teardown] are used in the same test suite, [Setup] or [Teardown] are executed instead of the universal Test Setup or Test Teardown. This can create unintended consequences if not done on purpose. This has been done accidentally by multiple people on my team. If Robocop recognized this as at least informational, it would be helpful.

sparkymartin avatar Aug 24 '22 16:08 sparkymartin

Do you mean this case?

*** Settings ***
Test Teardown    Keyword1

*** Test Cases ***
Test
    Pass

Test 2
    [Teardown]    Keyword2  # Keyword2 executes instead of Keyword1

bhirsz avatar Aug 24 '22 17:08 bhirsz

Yes...that case. And the same case, but for Test Setup and [Setup].

*** Settings *** Test Setup Keyword1

*** Test Cases *** Test Pass

Test 2 [Setup] Keyword2 # Keyword2 executes instead of Keyword1

sparkymartin avatar Aug 24 '22 17:08 sparkymartin

This is expected behaviour of the Robot Framework - how it allows to overwrite suite wide settings on test case level. So it don't break any rule. Hovewer I see the need to be informed on such cases if someone does not use that feature - I will take a look into it. But since it would not be default rule (only available if someone enables it) we will also need to implement default/non default rules feature in Robocop. So far all rules all enabled by default and it could be convenient to have some disabled by default.

bhirsz avatar Aug 24 '22 17:08 bhirsz

Another option would be to have it be a default rule, but informational (INFO instead of WARN). I like the idea of having a rule to flag this because it can easily be unintentionally misused...as I have encountered many times.

sparkymartin avatar Aug 24 '22 17:08 sparkymartin

Well, I think there's nothing here to be worried about in such cases, because it's how RF works. I would say that there should not be the rule like this, but since it can be INFO level, maybe it's worth trying. Also, by default it will not break the CI, because INFO messages do not affect the return code.

At the same time, I think that if someone wants and needs such rule in his/her project, one may just write it and use it :) I would not add a new disabled-by-default feature to Robocop just for the sake of such low-critical rule. So we can either add it as always-turned-on but with INFO level or do not add it at all and let the community write and use it on their own.

I need to think about it...

mnojek avatar Aug 26 '22 06:08 mnojek

You are right, it does actually sound like a perfect fit for external/custom rule. With that we dont need to introduce any news mechanism. If you want I can later prepare POC in this thread of such rule - you will be able to use it with robocop.

bhirsz avatar Aug 26 '22 06:08 bhirsz

I agree. It should be custom rule.

One thing I just came up with - with such a big community of Robocop users, maybe it would be good to create a place where people can exchange their custom rules? I was thinking about a separate repo (or maybe we can store it in ours, but maybe it's not the best idea) or some kind of storage. Very vague idea at the moment, but worth considering.

mnojek avatar Aug 26 '22 06:08 mnojek

Good idea on separate repository with custom rules. We could bootstrap it and add our test framework, deploy mechanism etc so it would be easy to add your own custom rules that could be installed by other users (ie with pip install robotframework-robocop[extra-rules] if we put this separate pip module in our extra profile).

bhirsz avatar Sep 15 '22 18:09 bhirsz

And that is the way we can take Robocop to the next level! I like it. Let's meet and discuss it offline.

mnojek avatar Sep 16 '22 06:09 mnojek

The idea from the comments has gained its own ticket #809, so I'm closing this one, since the original issue would not be implemented in the core Robocop.

mnojek avatar Apr 01 '23 11:04 mnojek