WPS437 Protected attribute usage (within the test directory)

[cidrblock]

This one caught my attention because it is from a test.

I see a few choices here,

• make it public but it's not really
• Don't test it the way it, but I think the test is needed
• add an exception in the tests, I could see this happening all over
• Make an exception for this in the tests directory

 Check warning on line 86 in tests/unit/actions/test_exec.py 

@ansible-zuulansible-zuul
/ ansible/check
tests/unit/actions/test_exec.py#L86
lint-vetting: WPS437 Found protected attribute usage: _generate_command

@ansible/devtools thoughts?

[webknjaz]

The "public API" concept can be applied at different levels: module, subpackage, package. It's often possible to restructure the code to improve the separation of concerns/abstraction layer distinction. If that's not possible, it's also possible to have some sort of a "proxy" module in tests that would import private things for testing and re-expose them as public, for example. Then, just one place would have to be ignored. I can't really add more specific ideas without seeing the actual code snippet where this is happening, there's not enough context for me.

[ssbarnea]

I think that for unittest we don't need to follow the same rules as for our code. In general I would find acceptable to have rule exceptions for unittests in order to make it easier to write them.

Use of private methods is acceptable inside unittests as we may need to access little bit more than the public method in order to obtain a good test coverage.

PS. I am not saying that we do not need to lint tests, just that I find ok to decide lowering the standards for them. It is far more important to enforce good standards on runtime code. Obviously that this should be decided on one-by-one evaluation.

[webknjaz]

Agreed, tests may need a more relaxed way of linting but sometimes these violations may reveal design problems because tests often demonstrate "how the code is used/called" by external parties (even if the external party is another submodule in the project). It's basically your first "API user". If you're hitting this in tests, ask yourself — is it possible to test this thing with an integration test?

[webknjaz]

@cidrblock as for the files — they don't exist in Git, probably because they are only committed to some branch that is not yet merged. This is why I'm asking for more specific links.

[cidrblock]

Oops, they are in the exec PR.

[webknjaz]

comment = command_test_data, command, additional_params

This does not have protected attribute access. Maybe the line changed over time as it happens with "sliding" Git labels, like branches.

[cidrblock]

yeah, it's gone now.

There are others like this one:

https://github.com/ansible/ansible-navigator/blob/main/tests/unit/actions/test_run.py#L190

and more I suspect.

[webknjaz]

@cidrblock would you be up to looking into a few of these cases on a call to figure out together if they could be improved w/o disabling the rule?