[Number of Public Attributes Check] Should ignore local test doubles

[obr-sap]

💡 Code pal for ABAP follows the Clean ABAP. If the issue relates to coding style, please submit it here.

Check Name
Number of Public Attributes Check

Actual Behavior
The current check also covers and reports findings for local test doubles, like the following:

CLASS ltd_my_double DEFINITION FOR TESTING.

  PUBLIC SECTION.
    INTERFACES if_my_interface.

    DATA mv_my_injected_attribute TYPE string.
ENDCLASS.

As an explanation: According to this test design pattern, the double behavior is controlled by these attributes. In addition, getter/setters are not needed due to the locality and the scope of the test fixture.

Still, the application of this pattern is reported by the check.

Expected Behavior
The check should ignore local classes for testing.

[pokrakam]

Interesting comment, tough one to call as there's also the argument that test code should be of the same standard as product code.

Personally I have never had the need to create many public attributes for test doubles so I don't see this as an issue, or it may even be a good thing to prompt one to consider the overall design?

Among the approaches I use are factory methods on the double class:
ltd_my_double=>get_instance( some_value = 1 ).
or factory injection
zcl_dependent_class_factory=>set_instance( ltd_my_double=>get_fail_case( ) ).

If the double hits the public attribute limit then maybe the original class is too big or we need more interfaces?
Also see some thoughts here.

[obr-sap]

Although I agree with the viability of the approaches and the argument about test code sharing the same standard in general, there is one aspect that shouldn't be neglected from point of view.

Information hiding (and with it decoupling) is the core motivation for avoiding public attributes. The question is whether such decoupling is desirable/needed for a local test double that can only be used by test classes in the same test include. This might be the case if the test include is very long (but then we'd have a smell of an overcomplex class, wouldn't we?). Otherwise, I'd argue that such decoupling is not desirable.

To put this into perspective: The pattern of declaring local friends from the test class to the main class is currently not disallowed (to my knowledge). This leads to the loss of information hiding of the main class in the context of the local test class. It is unclear to me why local test doubles should be better protected from local test classes than the main class (=productive code).

[thomham]

I second the comment from @obr-sap and would even change the title to "Should ignore classes marked with FOR TESTING".
The workaround would be to create getter and setter methods, but this would make the tests code unnecessary longer.
It is also not supported to create them automatically by the IDE, thus it means more typing.
Most important is the last point @obr-sap makes, test classes have the ability via LOCAL FRIEND concept to remove all access protection from the global class, thus they are already special and the argument they should adhere to the same standard is negated. Don't get me wrong, test classes should be clean and readable as product code. But this check should be disabled for FOR TESTING classes.

[pokrakam]

Over time I have gone from being a FRIENDs fan to steering away from class friendships and now try to use them sparingly. I have spent way too much time refactoring tests, and have found that testing if_cut instead of cl_cut makes unit tests far more maintainable.
The more we work with the internals the more brittle test classes become, increasing the amount of maintenance needed when refactoring the main class. And in my experience the same applies to test doubles.

A nice approach I picked up from the ABAP TDD book is to push some of the test functionality into the test double. Why query an attribute when the double can also double up (pun intended) as a spy? It can make it more reusable and readable with things like:

METHOD test_something. 
  data(double) = ltd_dependant=>create( ).
  zcl_dependant_factory=>inject( double ).
  cut->foo( ).
  double->assert_bar_was_called( ).
  double->assert_moo_not_zero( ). 
  double->assert_boo_less_than( c_max ).
ENDMETHOD.

It's not so much about information hiding but isolation from change and minimising levels of abstraction within a method. ltd_dependant above can become a global test class without much fuss, but not so easy with LOCAL FRIENDships.

PS.: @thomham getter and setter quick fixes have been available since at least 7.52:

[obr-sap]

@pokrakam It turns out that our point of view is the same with respect to :

• avoiding local friends
• typing cut as interface

I think the issue here is consistency of CodePal - why allow local friends but not public attributes of local doubles? In my opinion, the perception of CodePal by developers will be driven by consistency or the lack thereof. A less controversal approach would be to focus on undebatable check configurations first. In this case, this would mean restricting the public attributes check to global classes and still allowing local friends for the time being (although smelly).

As far as getter/setter are concerned, proper parameterized constructors (for stubs) and assert methods (for spies - as in your example) would be better alternatives to generated setters / getters respectively. So I wouldn't recommend to become lazy here by just generating some boilerplate code.

[pokrakam]

@obr-sap yes I think we are broadly in agreement on TDD principles.

However I don't really see it as inconsistent to allow LOCAL FRIENDs, they have their time and place. The same goes for FRIENDs in general: Mostly smelly, but a really good use case is factory injection.

The most obvious use case for LOCAL FRIENDS I can think of is to get legacy code under test. But here too I don't think it it necessary to have loads of attributes or otherwise treat test classes as special. Create the invasive test construct and build the tests on the back of it as usual.

I have ignored 'classic' ATC errors in test classes in the past, but have come around to the idea of keeping test code to the same standards as product code. This is just my experience, and I am always open to use cases where alternate guidelines make sense. One of the ways ABAP is special is that there are many exceptions, a recent example is #443.

As I said in my first comment, this is a tricky one to call. Maybe ignoring test classes should be an option, but I fear also it risks making at least a minority of people lazy/hasty/write less robust code. It's a catch-22: If too tightly coupled tests become too hard to maintain (I learnt that the hard way!) then people get put off tests. But having a high standard to start with puts people off writing tests in first place.
There's an excellent talk about this: TDD, Where Did It All Go Wrong. It's an hour long but well worth taking time to watch all the way through.

BTW I wasn't suggesting we should be blindly creating get/setters, that bit was specifically in response to the comment that this is not automated by the IDE.

[lucasborin]

@obr-sap: Do you have any examples in the SAP's internal systems? I would like to use it to go deeper into the subject before providing my perspective and recommendations.

[lucasborin]

How about adding a new flag in the Profile to inform if the Check is relevant for Class-Relevant Local Types and/or Local Types?

Would it support you @obr-sap?

[obr-sap]

I think the debate is only open for classes in the local test class include. For all other cases, there is a consensus to avoid public members.

[gerhard-dierkes-sap]

I think is is compatible with good programming style to have a couple of public attributes in local test double classes.
There is no need to encapsulate the properties of the test double instance.
So I support the request of the present issue.