Trying to add ABAP unit tests to a global class but could do with some help

[BaerbelWinkler]

Hi Folks,

as some of you know I'm struggling with a lot of ABAP OO stuff and am therefore quite happy that I at least managed to create a global class in a sandbox system doing what it's supposed to do, namely performing some checks before DDIC-objects get added to a transport. Here is the outline of the class and its methods:

What I'd like to do now is to add a test class in order to unit test the various methods in the class. So, yes, I didn't create the code via TDD as that wouldn't have ended well due to my basic struggles with OO as it would have added way too much complexity due to things I still don't understand and find confusing. However, as the logic will be in a pretty central and critical location I do see the advantage to have unit tests to ensure that future changes do not break the code.

For some methods this should be fairly straightforward as they only need to verify the import parameters and return abap_false or abap_true dependend on their content. I think (hope?) that I can create those routines with the help of various documents found online (e.g. 93b1c542d4984f0e9a75a57ce6030ff3's blog post, several posts by jjamese.mcdonough and others) and in books (e.g. bfeeb8ed7fa64a7d95efc21f74a8c135's chapter 5 from ABAP to the Future ed. 3). The latter also explains why it's quite okay to introduce FRIENDS in order to allow unit tests for private methods, something I wholeheartedly agree with! I already tried to make sure that the methods get all the information they need via importing parameters instead of directly from fields like sy-uname, sy-sysid and others in SYST so that the unit tests can work with other values than these (or wouldn't that have been necessary at all?).

What I am struggling with is what to do about the two routines which read TADIR (GET_SRCSYSTEM_FROM_TADIR) and a Z-Table (ENTRY_EXISTS_ZBC_DDIC_CHECK). I know that I'll have to somehow "trick" the unit test to play with provided data instead of actually reading the table, but I'm stymied of how to properly tweak my code to make it so.

From reading Paul's chapter, I gather that as a first step, I should move these two methods into a global class of their own where they'll then be public instead of private. Where I get lost is with deciding of what I need to define/code where and why. So, what goes in the test class and how do I need to rewrite the production code to allow "mocking" (or whatever the correct term is)? The thing I think I'm most confused about is where interface definitions for the global class(es) come in to allow testing. Is this a "must have" or is it just a "nice to have" and if the former how & where do I need to do this?

Not sure which code-snippets are needed to answer these questions, so here are some as an example for the SELECT from TADIR:

private definitions:

  PRIVATE SECTION.
    ......
    METHODS source_system_okay_for_ddic
      IMPORTING
        i_transport_objects            TYPE tr_objects
        i_sysid                        TYPE sy-sysid
      EXPORTING
        e_message                      TYPE bapiret2
      RETURNING
        VALUE(r_correct_source_system) TYPE abap_bool.
    .....
    METHODS get_srcsystem_from_tadir
      IMPORTING
        i_transport_object  TYPE e071
      RETURNING
        VALUE(r_srcsystem) TYPE tadir-srcsystem.

from main routine

"Now check that DDIC-system is source system of all objects in transport via TADIR
IF source_system_okay_for_ddic( EXPORTING i_transport_objects = i_transport_objects
                                          i_sysid             = i_sysid
                                IMPORTING e_message = message ).
  "All good and transport can be saved with objects
ELSE.
  "At least one object in transport has not been migrated to DDIC-system yet and message
  "information was filled in method accordingly
  APPEND message TO r_message.
ENDIF.

from SOURCE_SYSTEM_OKAY_FOR_DDIC

  METHOD source_system_okay_for_ddic.
    "----------------------------------------------------------------------
    " Method SOURCE_SYSTEM_OKAY_FOR_DDIC (private)
    "----------------------------------------------------------------------
    "   IMPORTING i_transport_objects TYPE tr_objects
    "             i_sysid             TYPE sy-sysid
    "   EXPORTING e_message           TYPE bapiret2
    "   RETURNING VALUE(r_correct_source_system)  TYPE abap_bool
    "----------------------------------------------------------------------
    "Default assumption is that system is okay for DDIC-maintenance
    r_correct_source_system = abap_true.
    CLEAR e_message.

    LOOP AT i_transport_objects INTO DATA(transport_object).
      .....
      DATA(lv_srcsystem) = get_srcsystem_from_tadir( transport_object ).

      IF lv_srcsystem NE space.
        ....
      ENDIF.

    ENDLOOP.

  ENDMETHOD.

  METHOD get_srcsystem_from_tadir.
    "----------------------------------------------------------------------
    " Method ENTRY_EXISTS_ZBC_DDIC_CHECK (private)
    "----------------------------------------------------------------------
    "   IMPORTING i_transport_object        TYPE e071
    "   RETURNING VALUE(r_srcsystem)        TYPE tadir-srcsystem
    "----------------------------------------------------------------------
    CLEAR r_srcsystem.
    SELECT SINGLE srcsystem
             INTO @r_srcsystem
             FROM tadir
            WHERE object   = @i_transport_object-object
              AND obj_name = @i_transport_object-obj_name(40).
  ENDMETHOD.

Any takers to help with this? And please let me know if this is not the best place to ask.

Thanks & Cheers

Bärbel

[FredericGirod]

Hi,

I think you have issue because you does not follow strictly the clean code. TDD/Abap Unit is really more simple when you follow clean code.

So, as you mention, Unit Test are only for Public method. And Public methods should only be available through interface: SOLID

And (my experience said) when you enter in this reasoning, you find that every private method could be extract to another class where they will be public. And in this new class you will be able to test them.

Why we should not test private method: Because the responsability of the class is only the public method. The private method is only here to help the public method to respond. And all the logic around the class: Interface, factory, ... are here to allow you to refactor the code without impacting the client class(the code using your class). If you put test on the private method you will broke that.

Normally Unit Test must not change if you create, delete, rename ... private method.

And finaly, try to split the Database part, it will help you to create mock and to be able to test it.

(sorry if I miss somethimg, your post was little bit long)

Fred

[gabmarian]

I agree, that only public methods should be tested which are the interface of the object exposed to the "outside world". The private sections are the "under the hood" part of the class which can be changed as time goes on.

If you feel that private methods need to be tested its a good sign to refactor the code into multiple classes. In this way you avoid the creation of God-objects

[matt]

It's generally true but...

sometimes I have complex bits of logic that are so tightly coupled to the logic that it doesn't make sense to make a global class. I could make a local class to encapsulate that bit, but sometimes, I just take a pragmatic short cut. 🙂

[BaerbelWinkler]

frdric.girod

Hi Fred,

thanks for your response! I'm well aware that I'm not following clean code but I did that on purpose as I didn't want "the perfect to be the enemy of the good". For me and my still spotty understanding of ABAP OO going full clean code and TDD would translate into never really getting the code done as the learning curve would be way too steep. I also prefer to understand what I'm implementing and not just copy and paste some "mysterious" code snippet from somewhere. Which is why I'm taking it in steps and being pragmatic and realistic of what I can do.

Cheers

Bärbel

[gabmarian]

Nice thing about interfaces is that they can be implemented by more than one classes. If you use an interface typed object in your code this means ANY object will suffice that implements that interface.

So let's say you want to get the system info from TADIR, which you want to use in the check:

INTERFACE zif_tadir_access.

  METHODS get_source_system
    IMPORTING
      pgmid      TYPE tadir-pgmid
      object     TYPE tadir-object
      obj_name   TYPE tadir-obj_name
    RETURNING
      VALUE(source_system) TYPE tadir-srcsystem.

ENDINTERFACE.

The logic here to determine the source system is "outsourced". In your check class you will use an object that implements that interface.

CLASS zcl_transport_checker DEFINITION. 
  PUBLIC SECTION.  
	METHOD run.
ENDCLASS.
...
METHOD run.

  " At this point the reader object is already provided
  DATA(source_sytem) = tadir_reader->get_source_system( ....  ).

ENDMETHOD.

Of course you also need a class implementing the real business logic doing the required stuff:

CLASS zcl_real_tadir_reader DEFINITION.

  PUBLIC SECTION.

    INTERFACES zif_tadir_access.

ENDCLASS.

CLASS zcl_real_tadir_reader IMPLEMENTATION.

  METHOD zif_tadir_access~get_source_system.
    SELECT SINGLE srcsystem
      FROM tadir
      INTO @ret
      WHERE pgmid    = @pgmid
        AND object   = @object
        AND obj_name = @obj_name.
  ENDMETHOD.

ENDCLASS.

Next part is to somehow provide the depended-on object for the checker, for example using constructor injection .

CLASS zcl_transport_checker DEFINITION. 

  PUBLIC SECTION.

    METHODS constructor
      IMPORTING
        tadir_reader TYPE REF TO zif_tadir_access OPTIONAL.

...
  METHOD constructor.

    IF tadir_reader IS SUPPLIED.
      me->tadir_reader = tadir_reader.                 " Take what we got
    ELSE.
      me->tadir_reader = NEW zcl_real_tadir_reader( ). " Use the default implementation
    ENDIF.

  ENDMETHOD.

In this example when you instantiate the checker, you accept a reader provided by the caller, but not necessarily expect one: if nothing is provided you stick to your business logic as a default implementation. In productive use of the check class it will be OK not to provide anything.

Now, everything is set to write unit tests.

You can create a fake class that will mock the TADIR access:

CLASS lcl_fake_tadir_reader DEFINITION.

  PUBLIC SECTION.
    INTERFACES zif_tadir_access.

ENDCLASS.

CLASS lcl_fake_tadir_reader IMPLEMENTATION.
  METHOD zif_tadir_access~get_source_system.
    ret = 'DUMMY'.
  ENDMETHOD.
ENDCLASS.

And in your test method inject the fake reader when you create the checker:

METHOD test.
  DATA(cut) = NEW zcl_transport_checker( tadir_reader = lcl_fake_tadir_reader( ) ).
ENDMETHOD.

It ended up a bit lengthy answer, but I hope it makes more sense now. 🙂

[BaerbelWinkler]

Thanks for your helpful response, Gabor! At first read I think I get at least the gist of it, which will have to do for the moment. The real "test" will come once I try to actually update my code.

Cheers

Bärbel

[matt]

I'll take your method get_srcsystem_from_tadir.

First define it in a global interface ZIF_DAO. Then implement it in ZCL_DAO. (DAO stands for data access object - I use this convention in most of my programs. I've also renamed your methods and variables to my preferences.

INTERFACE ZIF_DAO.
  METHODS: 
    get_srcsystem_from_tadir
      IMPORTING
        i_transport_object TYPE e071
      RETURNING
         VALUE(r_result)   TYPE tadir-srcsystem.
ENDINTERFACE.

CLASS ZCL_DAO DEFINITION.
  PUBLIC SECTION.
    INTERFACES zif_dao.
ENDCLASS.

CLASS ZCL_DAO IMPLEMENTATION.
  METHOD zif_dao~get_srcsystem_from_tadir.
    SELECT SINGLE srcsystem
             INTO @r_result
             FROM tadir
            WHERE object   = @i_transport_object-object
              AND obj_name = @i_transport_object-obj_name(40).
  ENDMETHOD:
ENDCLASS:

In my calling class, I create a static attribute _dao type zif_dao.

In my calling class, I put this into the CLASS_CONSTRUCTOR

_dao = new zcl_dao( ).

And where I would have had my select, I have instead

src_system = _dao->get_srcsystem_from_tadir( ).

Now for the unit testing. Create a local test double

CLASS ltd_dao DEFINITION.
  PUBLIC SECTION.
    INTERFACES zif_dao.
    DATA:
      source_system TYPE tadir-srcsystem.
ENDCLASS.

CLASS ltd_dao IMPLEMENTATION.
  METHOD zif_dao~get_srcsystem_from_tadir.
    r_result = source_system.
  ENDMETHOD:
ENDCLASS:

Now for the unit test

CLASS ltc_dao DEFINITION FOR TESTING 
              RISK LEVEL HARMLESS
              DURATION SHORT.
  PRIVATE SECTION.
    DATA:
      cut TYPE REF TO <the_global_class>,
      dao TYPE REF TO ltd_dao.
    METHODS:
      setup,
      get_tp_source_system.
ENDCLASS.
CLASS ltc_dao IMPLEMENTATION.
  METHOD setup.
    cut = new #( ).
    dao = new #( ).
    cut->_dao = dao. " This is known as injection
                     " Now your ltd_dao will be
                     " used instead of CL_DAO
  ENDMETHOD.

  METHOD get_tp_source_system.
    dao->source_system = 'TEST'.
    cl_abap_unit_assert=>assert_equals(
              exp = 'TEST'
              act = cut->get_srcsystem_from_tadir( ). 
  ENDMETHOD.
ENDCLASS.

You need to make ltc_dao a local friend of your global class. You do this in the class relevant local types of your global class.

CLASS ltc_dao DEFINITION DEFERRED.
CLASS <the global class name> DEFINITION LOCAL FRIENDS ltc_dao.

Now you can run your unit test, and instead of reading the database via cl_dao it returns the result set up in ltd_dao.

I use a DAO class like this for any db reads or calling any sap functions or methods.

Any questions?

[matt]

Gabor's approach is another way of injecting the test double. Though I never create global test classes. Injection is useful not just for testing, but whenever you want to use a different implementation of the same interface - q.v. "strategy pattern".

[FredericGirod]

You do not follow the Given / When / Then ?

I am little bit more school, and I strictly follow it, mostly for the further reader

[matt]

No. I don't follow given/when/then - I've heard of it, but not really grasped it. How would you redo my test class so it does follow it?

[gabmarian]

matthew.billingham

There is a pretty nice explanation by Martin Fowler:

1. The given part describes the state of the world before you begin the behavior you're specifying in this scenario. You can think of it as the pre-conditions to the test.
2. The when section is that behavior that you're specifying.
3. Finally the then section describes the changes you expect due to the specified behavior.

Applying this to your case, when you are tesing whether setting the source system to 'TEST' has the desired effect:

  METHOD get_tp_source_system.
    "When 
    dao->source_system = 'TEST'.
    "Then
    cl_abap_unit_assert=>assert_equals(
              exp = 'TEST'
              act = cut->get_transport_source_system( ). 
  ENDMETHOD.

No "Given" part here, as there are no assumptions before the assignment.

[FredericGirod]

Not totally agree with you Gábor Márián

for me it is

  METHOD get_tp_source_system.
    " Given I have a test system
    dao->source_system = 'TEST'.
    "When I regest the source system
    data(result) = cut->get_transport_source_system( )
    " Then it should reply 'TEST'
    cl_abap_unit_assert=>assert_equals(
              exp = 'TEST'
              act = result . 
  ENDMETHOD.

[gabmarian]

frdric.girod

I think it's up to the decision what the behavior "being observed" is. In my case this is changing the source system, and in your case this this is the call of the getter method. Looks like it's not trivial even in the simplest of cases. 🙂

[matt]

@Gabor Marian, frdric.girod

Thanks very much. That makes it clear. I guess I've been over thinking it!

[BaerbelWinkler]

matthew.billingham

Thanks for your response, Matt! I'd be surprised if I don't come up with some questions once I start tweaking my code based on your and and Gábor's suggestions and provided code snippets!

Cheers

Bärbel

[BaerbelWinkler]

matthew.billingham

Hi Matt!

I knew it wouldn't take long until I had a first question, so here it is:

Where / how do I need to add these statements?

"In my calling class, I create a static attribute _dao type zif_dao."

"In my calling class, I put this into the CLASS_CONSTRUCTOR"

==> I tried various places and ended up putting the attribute into CLASS_CONSTRUCTOR where it at least didn't produce a syntax warning. I think it also needs the "REF TO" addition - at least according to the mesage I got at frst.

==> The CLASS_CONSTRUTOR looks like this now (I decided to use different names than what you suggested to show where the interface and dao-class "belong to"):

STATICS: _dao TYPE REF TO zif_check_wb_action_dao.

_dao = new zcl_check_wb_action_dao( ).

"And where I would have had my select, I have instead"

src_system = _dao->get_srcsystem_from_tadir().

==> I changed the code to this

*      DATA(lv_srcsystem) = get_srcsystem_from_tadir( transport_object ).
      DATA(lv_srcsystem) = _dao->get_srcsystem_from_tadir( ).

But this now gives me a syntax error for _dao:

So, if I had to guess, I didn't pick the right place and/or syntax to define _dao.

Cheers

Bärbel

[FredericGirod]

Bärbel Winkler could you poste the whole code (the interesting part) because it is difficult to follow the logic

[BaerbelWinkler]

frdric.girod

Hi Fred,

not sure if this is what you mean but here is what I created based on Matt's suggestion:

New Interface:

interface ZIF_CHECK_WB_ACTION_DAO
  PUBLIC.

  METHODS:
    get_srcsystem_from_tadir
      IMPORTING
        i_transport_object TYPE e071
      RETURNING
        VALUE(r_srcsystem)    TYPE tadir-srcsystem.

endinterface.

New global class for the data access:

CLASS zcl_check_wb_action_dao DEFINITION
  PUBLIC
  FINAL
  CREATE PUBLIC .

  PUBLIC SECTION.

    INTERFACES zif_check_wb_action_dao.

  PROTECTED SECTION.
  PRIVATE SECTION.
ENDCLASS.

CLASS zcl_check_wb_action_dao IMPLEMENTATION.

  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.
    "----------------------------------------------------------------------
    " Method GET_SRCSYSTEM_FROM_TADIR (ppublic)
    "----------------------------------------------------------------------
    "   IMPORTING i_transport_object        TYPE e071
    "   RETURNING VALUE(r_srcsystem)        TYPE tadir-srcsystem
    "----------------------------------------------------------------------

    CLEAR r_srcsystem.

    SELECT SINGLE srcsystem
         INTO @r_srcsystem
         FROM tadir
        WHERE object   = @i_transport_object-object
          AND obj_name = @i_transport_object-obj_name(40).

  ENDMETHOD.

ENDCLASS.

Updated section in my existing global class for the checks:

CLASS zcl_check_wb_action DEFINITION
  PUBLIC
  FINAL
  CREATE PUBLIC .

  PUBLIC SECTION.

    CLASS-METHODS: class_constructor.
....

Snippets from the implentation (this is where I'm not sure if the STATICS-statement is where it should be):

CLASS zcl_check_wb_action IMPLEMENTATION.

  METHOD class_constructor.
    "----------------------------------------------------------------------
    " Method CLASS_CONSTRUCTOR (public)
    "----------------------------------------------------------------------
    "   Preparation to make unit testing possible via dao-class and interface
    "----------------------------------------------------------------------

    "Static attribute to refer to the interface for the data access objects (DAO) class
    STATICS: _dao TYPE REF TO zif_check_wb_action_dao.

    _dao = new zcl_check_wb_action_dao( ).

  ENDMETHOD.

  METHOD source_system_okay_for_ddic.
    "----------------------------------------------------------------------
    " Method SOURCE_SYSTEM_OKAY_FOR_DDIC (private)
    "----------------------------------------------------------------------
    "   IMPORTING i_transport_objects TYPE tr_objects
    "             i_sysid             TYPE sy-sysid
    "   EXPORTING e_message           TYPE bapiret2
    "   RETURNING VALUE(r_correct_source_system)  TYPE abap_bool
    "----------------------------------------------------------------------

    "Default assumption is that system is okay for DDIC-maintenance
    r_correct_source_system = abap_true.
    CLEAR e_message.

    LOOP AT i_transport_objects INTO DATA(transport_object).

      "Need to determine source system via TADIR
      "Lookup for OBJECT = STRU needs to be done for TABL
      IF transport_object-object EQ 'STRU'.
        transport_object-object = 'TABL'.
      ENDIF.

      DATA(lv_srcsystem) = get_srcsystem_from_tadir( transport_object ).
*     DATA(lv_srcsystem) = _dao->get_srcsystem_from_tadir( ).

The active line is the one which works in the existing logic, the commented out one gives a syntax error if it's active instead of the one above.

I'm sure this is due to me not really understanding what I'm supposed to be doing!

[FredericGirod]

Juste switch the position of the line:

STATICS: _dao TYPEREFTO zif_check_wb_action_dao.

in the class declaration, because, you have declare this ref inside a method. So it is local

here, it will be global for the class

CLASS zcl_check_wb_action DEFINITIONPUBLICFINALCREATEPUBLIC.

PUBLICSECTION.
statics: _dao TYPEREFTO zif_check_wb_action_dao.
CLASS-METHODS: class_constructor.

[BaerbelWinkler]

frdric.girod

I had tried your suggestion, but it doesn't like "statics" there and throws a syntax error "The statement STATICS is unexpected". What does work from a syntax point of view is this:

CLASS zcl_check_wb_action_dao DEFINITION
  PUBLIC
  FINAL
  CREATE PUBLIC .

  PUBLIC SECTION.

    CLASS-DATA _dao TYPE REF TO zif_check_wb_action_dao.

    CLASS-METHODS: class_constructor.

    INTERFACES zif_check_wb_action_dao.

Update: I'm now also getting confused which code I need to add in which class - the unit testing for the TADIR-Select needs to be done in the the newly created class ZCL_DAO in Matt's example which I named ZCL_CHECK_WB_ACTION_DAO as each global class has it's own test class, right? But where all do I have to now work via the interface ZIF_CHECK_WB_ACTION_DAO and add the CLASS-DATA and CLASS-CONSTRUCTOR? In both or just the DAO-one?

I also had to tweak the call in the relevant method of my initial class (ZCL_CHECK_WB_ACTION) to include the importing parameter as it didn't accept the empty brackets Matt had in his code examples:

DATA(lv_srcsystem) = _dao->get_srcsystem_from_tadir( tranpsort_object ).

[matt]

This is what your class using zcl_dao should look like

CLASS my_class DEFINITION.
  PUBLIC SECTION.
    INTERFACES zif_dao.
    CLASS-METHODS:
      class_constructor.
...
  PRIVATE SECTION.
    CLASS-DATA:
      _dao TYPE REF TO zif_dao.
...
ENDCLASS.

CLASS my_class IMPLEMENTATION.
  METHOD class_constructor.
    _dao = NEW zcl_dao( ).
  ENDMETHOD.

  METHOD source_system_okay_for_ddic.
    ...
    DATA(lv_srcsystem) = get_srcsystem_from_tadir( transport_object ).
    ...
  ENDMETHOD:
ENDCLASS.

ltd_dao, ltc_dao are local classes of my_class. ltc_dao is a local friend of my_class.

But I think you were pretty close.
The term class methods, class attributes/date and static methods, static attributes, are interchangeable. In the form based class editor, they are labelled as "static". The keyword statics can't be used in a class. You use class-data instead.

I hope that helps clarify things. I'm writing this code directly, so errors are possible - like not providing a parameter in the method call! 🙂

The test class setup sets _dao in my_class directly to be (an instance with reference to) ltd_dao. This is also a form of injection. It's a matter of preference. Any methods called on _dao will run the code in ltd_dao rather than that in ZCL_DAO.

Changing which implementation of an interface is called at runtime, without changing the code, is an example of polymorphism, which you may have heard of.

[BaerbelWinkler]

matthew.billingham

Thanks for bearing with me, Matt! I'll try to clean up my code tomorrow when I'm back in the office.

Just to clarify where what needs to go:

My main global class is ZCL_CHECK_WB_ACTION (what you show as my_class I guess?) which thus far has a private method source_system_okay_for_ddic from which the method get_srcsystem_from_tadir gets called from. The latter method will now move as a public method to a new global class which I called ZCL_CHECK_WB_ACTION_DAO. There's also the new interface ZIF_CHECK_WB_ACTION_DAO.

Will there (need to) be unit testing code for method get_srcsystem_from_tadir in ZCL_CHECK_WB_ACTION_DAO and/or will I need this "mocking" so that the unit tests for ZCL_CHECK_WB_ACTION itself work w/o access TADIR?

[BaerbelWinkler]

matthew.billingham

New day .... new questions!

Having the select from TADIR in new class ZCL_CHECK_WB_ACTION_DAO and using the interface ZIF_CHECK_WB_ACTION_DAO for the actual process works according to the debugging. I however get a syntax check warning about a missing implementation for ZIF_CHECK_WB_ACTION_DAO~GET_SRCSYSTEM_FROM_TADIR:

As it's just a warning I could activate the code regardless, but I'd like to get rid of the warning if possible.

I however also get an error for the test class which I don't know how to eliminate - at a guess "ZIF_CHECK_WB_ACTION_DAO needs to make an appearance somehow/where but the things I already tried didn't work:

Here is the complete test class:

*"* use this source file for your ABAP unit test classes
CLASS ltc_dao DEFINITION FOR TESTING
              RISK LEVEL HARMLESS
              DURATION SHORT.
  PRIVATE SECTION.
    DATA:
      cut TYPE REF TO zcl_check_wb_action,
      dao TYPE REF TO ltd_dao.
    METHODS:
      setup,
      get_tp_source_system.
ENDCLASS.

CLASS ltc_dao IMPLEMENTATION.
  METHOD setup.
    cut = new #( ).
    dao = new #( ).
    cut->_dao = dao. " This is known as injection
                     " Now your ltd_dao will be
                     " used instead of CL_DAO
  ENDMETHOD.

  METHOD get_tp_source_system.
    dao->source_system = 'TEST'.
    cl_abap_unit_assert=>assert_equals(
              exp = 'TEST'
              act = cut->get_srcsystem_from_tadir( ).
  ENDMETHOD.
ENDCLASS.

Class relevant local types:

CLASS ltc_dao DEFINITION DEFERRED.
CLASS zcl_check_wb_action DEFINITION LOCAL FRIENDS ltc_dao.

Local types:

CLASS ltd_dao DEFINITION.
  PUBLIC SECTION.
    INTERFACES zif_check_wb_action_dao.
    DATA:
      source_system TYPE tadir-srcsystem.
ENDCLASS.

CLASS ltd_dao IMPLEMENTATION.
  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.
    r_srcsystem = source_system.
  ENDMETHOD.
ENDCLASS.

[FredericGirod]

It is because you have declare CUT like that

cut TYPE REF TO zcl_check_wb_action,<br>

so if you want to access to a method declared in the interface you have to use the statement

  METHOD get_tp_source_system.
    dao->source_system = 'TEST'.
    cl_abap_unit_assert=>assert_equals(
              exp = 'TEST'
              act = cut->zif_check_wb_action_dao~get_srcsystem_from_tadir( ).
  ENDMETHOD.<br>

if you have been declared CUT type ref to the interface, you will not have this issue.

so the second solution is :

data cut TYPE REF TO zif_check_wb_action_dao.
cut = new zcl_check_wb_action(). 

[BaerbelWinkler]

frdric.girod & matthew.billingham

Thanks for your continued help!

I at least now managed to get a syntax-error free test class after adding some values for structure transport_object and passing that into the method call:

  METHOD get_tp_source_system.
    dao->source_system = 'TEST'.

    "DDIC-check
    transport_object-object   = 'DTEL'.
    transport_object-obj_name = 'DUMMY'.

    cl_abap_unit_assert=>assert_equals(
              exp = 'TEST'
              act = cut->zif_check_wb_action_dao~get_srcsystem_from_tadir( transport_object ) ).
  ENDMETHOD.<br>

But, when I run the unit test, it comes back with this error:

"'Method call failed; the method ZIF_CHECK_WB_ACTION_DAO~GET_SRCSYSTEM_FROM_TADIR of the class ZCL_CHECK_WB_ACTION is not implemented'"

Is this related to the syntax warning mentioned above?

This is all really confusing, so I may soon give up with this quite complex way to get unit testing into my class and give Mike's "baby-steps" approach a try.

[matt]

Will there (need to) be unit testing code for method get_srcsystem_from_tadir in ZCL_CHECK_WB_ACTION_DAO and/or will I need this "mocking" so that the unit tests for ZCL_CHECK_WB_ACTION itself work w/o access TADIR?

You'll be mocking it for ZCL_CHECK_WB_ACTION (my_class). Once you get down to the actual SQL, except in high versions of ABAP, there's no way to automate the unit test... safely.

As it's just a warning I could activate the code regardless, but I'd like to get rid of the warning if possible.

I use CTRL-1 and choose implement methods option. Later versions of ABAP you can use:

CLASS ltd DEFINITION FOR TESTING.
  INTERFACES zif_check_wb_action_dao INTERFACES PARTIALLY IMPLEMENTED.
,,,

[matt]

Which class does the "problems" tab say is missing the method implementation?

ZCL_CHECK_WB_ACTION_DAO should have the SQL in its implementation of ZIF_CHECK_WB_ACTION_DAO~GET_SRCSYSTEM_FROM_TADIR.

CLASS zcl_check_wb_action_dao IMPLEMENTATION.
  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.
    SELECT SINGLE srcsystem
         INTO @r_srcsystem
         FROM tadir
        WHERE object   = @i_transport_object-object
          AND obj_name = @i_transport_object-obj_name(40).
  ENDMETHOD.
ENDCLASS.

ltd_dao should have

CLASS ltd_dao IMPLEMENTATION.
  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.
    r_srcsystem = source_system.
  ENDMETHOD.
ENDCLASS.~~

[BaerbelWinkler]

matthew.billingham

Hi Matt,

this is what the Unit test result says:

"'Method call failed; the method ZIF_CHECK_WB_ACTION_DAO~GET_SRCSYSTEM_FROM_TADIR of the class ZCL_CHECK_WB_ACTION is not implemented'"

ZCL_CHECK_WB_ACTION_DAO does have the required code to read TADIR.

I've now used CTRL+1 to add an implementation for the apparently missing method in ZCL_CHECK_WB_ACTION, but don't have a clue if it can stay without anything in it or if some logic actually needs to be added:

  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.

  ENDMETHOD

The Unit test then doesn't run into a technical error but fails because the expected result doesn't match the actual result. This is not a surprise really with value "TEST" but no clue how to get it to green as the logic in the productive code requires two SELECTs in sequence, the first one to determine if the object to be added to the transport falls into the "DDIC-category". This gets done via entries in a Z-Table as I didn't find anything which makes this kind of decision based on Objecttype (i.e. DTEL = DDIC vs. PROG = ABAP). Only if this first check finds that a DDIC-object is impacted will the second check via TADIR be executed to check if the sourcesystem of the object is okay.

I'm afraid that I'm getting ever more confused at what needs to be done and - perhaps more importantly - why! I think that part of my confusion comes from not yet understanding which parts of the productive code actually get executed during unit testing. Is it anything else apart from the definitions and implementations of the method getting tested - meaning, unit tests don't care what happens before or after them and you have to feed them "mock" data which during actual execution gets determined by other logic/methods?

To me, it looks as if there isn't really an easy and practicle means to mock actual data access for the purpose of unit testing because it requires so much additional code that you (meaning "me"!) run the risk of breaking the productive code in order to add it in which doesn't really seem to be worth the effort.

[matt]

Because I've been doing this off-line, I've made a few errors, so that probably hasn't helped. ABAP Unit Tests is entirely practical, and once you've done it once, it isn't difficult. It took me time to really get the hang of it.

You are wanting to test your method that calls private method source_system_okay_for_ddic You've got all the bits, it's just a question of putting them together. Now I've actually got a system I can play with, here's my complete solution:

First, the interface for the data access object.

INTERFACE zif_check_wb_action_dao PUBLIC .
  METHODS:
    get_srcsystem_from_tadir
      IMPORTING
        i_transport_object TYPE e071
      RETURNING
        VALUE(r_result)    TYPE tadir-srcsystem.
ENDINTERFACE.

Now the concrete, global implementation that does the database access

CLASS zcl_check_wb_action_dao DEFINITION PUBLIC FINAL CREATE PUBLIC .
  PUBLIC SECTION.
    INTERFACES zif_check_wb_action_dao.
  PROTECTED SECTION.
  PRIVATE SECTION.
ENDCLASS.

CLASS zcl_check_wb_action_dao IMPLEMENTATION.
  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.
    SELECT SINGLE srcsystem
             INTO @r_result
             FROM tadir
            WHERE object   = @i_transport_object-object
              AND obj_name = @i_transport_object-obj_name(40).
  ENDMETHOD.
ENDCLASS.

Now the class with the method you want to test. I've called this main_routine. What we want to know is: if the object in the transport exists in tadir with a system defined, is the message returned empty. If no system exists or the record isn't in TADIR, then return an error message.

CLASS zcl_check_wb_action DEFINITION PUBLIC FINAL CREATE PUBLIC .
  PUBLIC SECTION.
    CLASS-METHODS
      class_constructor.
    METHODS:
      main_routine
        IMPORTING
          i_transport_objects TYPE tr_objects
          i_sysid             TYPE sy-sysid
        RETURNING
          VALUE(r_message)    TYPE bapiret2_t.
  PROTECTED SECTION.
  PRIVATE SECTION.
    CLASS-DATA _dao TYPE REF TO zif_check_wb_action_dao.
    METHODS source_system_okay_for_ddic
      IMPORTING
        i_transport_objects            TYPE tr_objects
        i_sysid                        TYPE sy-sysid
      EXPORTING
        e_message                      TYPE bapiret2
      RETURNING
        VALUE(r_correct_source_system) TYPE abap_bool.
    METHODS get_srcsystem_from_tadir
      IMPORTING
        i_transport_object TYPE e071
      RETURNING
        VALUE(r_srcsystem) TYPE tadir-srcsystem.
ENDCLASS.

CLASS zcl_check_wb_action IMPLEMENTATION.
  METHOD get_srcsystem_from_tadir.
    r_srcsystem = _dao->get_srcsystem_from_tadir( i_transport_object ).
  ENDMETHOD.

  METHOD source_system_okay_for_ddic.
    "Default assumption is that system is okay for DDIC-maintenance
    r_correct_source_system = abap_true.
    CLEAR e_message.
    LOOP AT i_transport_objects INTO DATA(transport_object).
      ...
      " Could call _dao->get_srcsystem_from_tadir directly. Matter of choice.
      DATA(lv_srcsystem) = get_srcsystem_from_tadir( transport_object ).
      IF lv_srcsystem NE space.
        ...
      else.
        r_correct_source_system = abap_false.
        e_message-message = 'All gone horribly wrong'.
      ENDIF.
    ENDLOOP.
  ENDMETHOD.

  METHOD main_routine.
    "Now check that DDIC-system is source system of all objects in transport via TADIR
    DATA message TYPE bapiret2.
    IF source_system_okay_for_ddic( EXPORTING i_transport_objects = i_transport_objects
                                              i_sysid             = i_sysid
                                    IMPORTING e_message           = message ).
      "All good and transport can be saved with objects
    ELSE.
      "At least one object in transport has not been migrated to DDIC-system yet and message
      "information was filled in method accordingly
      APPEND message TO r_message.
    ENDIF.
  ENDMETHOD.

  METHOD class_constructor.
    _dao = NEW zcl_check_wb_action_dao( ).
  ENDMETHOD.
ENDCLASS.

In the "Class-relevant Local Types" tab, you need to give the test class access to the class under test.

CLASS ltc_action DEFINITION DEFERRED.
CLASS zcl_check_wb_action DEFINITION LOCAL FRIENDS ltc_action.

And finally in the Test Classes tab:

*"* use this source file for your ABAP unit test classes
CLASS ltd_dao DEFINITION FOR TESTING.
  PUBLIC SECTION.
    INTERFACES zif_check_wb_action_dao.
    DATA:
      tadir TYPE STANDARD TABLE OF tadir WITH DEFAULT KEY.
ENDCLASS.

CLASS ltd_dao IMPLEMENTATION.
  METHOD zif_check_wb_action_dao~get_srcsystem_from_tadir.
    READ TABLE tadir WITH KEY object   = i_transport_object-object
                              obj_name = i_transport_object-obj_name(40)
                     INTO DATA(tadir_entry).
    r_result = tadir_entry-srcsystem.
  ENDMETHOD.
ENDCLASS.

CLASS ltc_action DEFINITION FOR TESTING DURATION SHORT RISK LEVEL HARMLESS.
  PRIVATE SECTION.
    DATA:
      cut TYPE REF TO zcl_check_wb_action,
      dao TYPE REF TO ltd_dao.
    METHODS:
      setup,
      main_routine_valid_object FOR TESTING RAISING cx_static_check,
      main_routine_invalid_object FOR TESTING RAISING cx_static_check,
      main_routine_sys_undefined FOR TESTING RAISING cx_static_check.
ENDCLASS.

CLASS ltc_action IMPLEMENTATION.

  METHOD setup.
    dao = NEW #( ).
    cut = NEW #( ).
    cut->_dao = dao. " Injection
    dao->tadir = VALUE #( ( object = 'OBJ_A' obj_name = 'NAME_A' srcsystem = 'SYS_A' )
                          ( object = 'OBJ_B' obj_name = 'NAME_B' srcsystem = 'SYS_B' )
                          ( object = 'OBJ_C' obj_name = 'NAME_C' srcsystem = '' )  ).
  ENDMETHOD.

  METHOD main_routine_valid_object.
    DATA(messages) = cut->main_routine( i_transport_objects = VALUE #( ( object = 'OBJ_A' obj_name = 'NAME_A' ) )
                                        i_sysid             = '' ).
    cl_abap_unit_assert=>assert_initial( messages ).
  ENDMETHOD.

  METHOD main_routine_invalid_object.
    DATA(messages) = cut->main_routine( i_transport_objects = VALUE #( ( object = 'OBJ_C' obj_name = 'NAME_C' ) )
                                        i_sysid             = '' ).
    cl_abap_unit_assert=>assert_not_initial( messages ).
    cl_abap_unit_assert=>assert_equals( act = messages[ 1 ]-message exp = 'All gone horribly wrong' ).
  ENDMETHOD.

  METHOD main_routine_sys_undefined.
    DATA(messages) = cut->main_routine( i_transport_objects = VALUE #( ( object = 'OBJ_C' obj_name = 'NAME_C' ) )
                                        i_sysid             = '' ).
    cl_abap_unit_assert=>assert_not_initial( messages ).
    cl_abap_unit_assert=>assert_equals( act = messages[ 1 ]-message exp = 'All gone horribly wrong' ).
  ENDMETHOD.
ENDCLASS.<br>

This is taken from a real system, and it works. The tests run green. In ltd_dao, I've set up an internal table to take the place of the database table. I then populate it in the setup, and test various scenarios, as visible from the name of the test methods.

I hope this resolves your difficulties seeing how to piece it together.

The refactoring (the abstraction of the db access into an interface/class) doesn't add much code. It improves your original code, because it makes it more modular and in fact more robust. The only substantive additional code is the unit tests and test doublesa and this can't screw up the productive code. It does take additional effort to implement unit tests, but the benefits are so huge, they completely offset that extra time.

[BaerbelWinkler]

matthew.billingham

Thanks Matt, for going to all this trouble to help me with this!

I'm currently checking and tweaking my code according to your comment. The interface and the DAO-class already look like they should, so I'm now working on ZCL_CHECK_WB_ACTION. What I don't quite understand is why you still have a method definition and implementation for GET_SRCSYSTEM_FROM_TADIR in there. Isn't this now obsolete for this class as the logic is in ZCL_CHECK_WB_ACTION_DAO and will get called via the interface? I no longer have it in my class and the logic works as it should.

    METHODS get_srcsystem_from_tadir
      IMPORTING
        i_transport_object TYPE e071
      RETURNING
        VALUE(r_srcsystem) TYPE tadir-srcsystem

And another question:

Could the defintion and implementation for the local test double LTD_DAO be put into the tab for local types instead of local test classes? Which is where I had comparable and just slightly different code before following some earlier suggestions. Are there "rules" of where which code should go where?

Next, I'll have to think about how to tweak the tests to make them fit our actual process. It's for example not an error if no entry is found in TADIR (and source system therefore stays empty) as that just means that we are dealing with a new object which can be created in the system as long as the system itself is okay for the handling of DDIC-objects (which is another check).

Cheers

Bärbel

[matt]

What I don't quite understand is why you still have a method definition and implementation for GET_SRCSYSTEM_FROM_TADIR in there.

I've fixed that now. My intention was to make minimal changes. Just change the code in ZCL_WB_CHECK_ACTION in the method, but leave the signature unchanged. The alternative is to call _dao->get_srcsystem_from_tadir( transport_object ) directly, but that involves, as you say, removing the method definition and implementation from ZCL_WB_CHECK_ACTION.

Could the definition and implementation for the local test double LTD_DAO be put into the tab for local types instead of local test classes? Which is where I had comparable and just slightly different code before following some earlier suggestions. Are there "rules" of where which code should go where?

Maybe you version is different, but my version of Eclipse/ADT has a tab for "Test Classes". So I put them there so people know where to find them. In "Local Types" I put any lcl_ classes - i.e. local classes I use from the global class productively.

[FredericGirod]

And there is another solution people mostly does not like.

In your test you inject data in the TADIR. (yes, and update)

in your TEARDOWN method, you delete it (just to be sure)

because, normally, there is no COMMIT inside the TDD.

[matt]

Would that still be RISK LEVEL HARMLESS? Now, if I was on 7.52...

[gabmarian]

This is kind of dangerous. If e.g. during debugging you issue a COMMIT and the processing dumps for some reason then you have created an inconsistency.

[FredericGirod]

And also it could be dangerous if inside the code tested you have a commit

[pokrakam]

I'm going to go a little bit against best practice and align more with "Baby Steps" principle in suggesting inheritance.

The goal here is not just to provide the best possible test, but also to ease the introduction to OO and unit testing. Usually any "dependent on" components should be split out and implemented as interfaces, but bringing this back into a simplest possible form we have:

1. a couple of public methods that we want to test, and
2. two private methods access stuff outside the class. They are the dependencies we want to remove.

So, subclassing is also a bit easier to get a handle on than interfaces, I would suggest making the two methods that create a dependency on data in tables protected. Then the test class can redefine them and supply it's own values for each test:

CLASS ltc_check_wb_action DEFINITION 
  INHERITING FROM zcl_check_wb_action
  FOR TESTING
  RISK LEVEL HARMLESS
  DURATION SHORT.
  
PROTECTED SECTION. 
  METHODS get_srcsystem_from_tadir REDEFINITION. 

PRIVATE SECTION.
  DATA srcsystem TYPE tadir-srcsystem.
  
  METHODS source_abc_ok FOR TESTING RAISING cx_static_check. 
  METHODS source_xzy_fails FOR TESTING RAISING cx_static_check. 
ENDCLASS. 


CLASS ltc_check_wb_action IMPLEMENTATION. 

  METHOD get_srcsystem_from_tadir. "Redefined
    "Each test can set the private attribute that this method should return
    r_srcsystem = srcsystem.   
  ENDMETHOD. 
  
  METHOD source_abc_ok.
    srcsystem = 'ABC'.  "Setup the value we want to pretend to find in TADIR (mock)
	"... create some dummy values if needed
    data(check_result) = check_wb_action( 
	    i_transport_objects = value #( ( ) )   "Table with an empty row to make it loop once
	    i_sysid = value #( ) ).
    cl_abap_unit_assert=>assert_true( check_result ).  "Assuming it's a boolean, else use assert_equals
  ENDMETHOD.
  
  METHOD source_xzy_fails. 
    srcsystem = 'XYZ'.  "Mock another system
    data(check_result) = check_wb_action( 
	    i_transport_objects = value #( ( ) )  
	    i_sysid = value #( ) ).
    cl_abap_unit_assert=>assert_false( check_result ).
  ENDMETHOD.

ENDCLASS.

We have made almost no change to the original code. This is also a good technique to get legacy code under test fast, and later refactor into separate Data Access Classes and all that.

The only thing to be wary of is that your constructor shouldn't do too much and especially shouldn't call any of the redefined methods. But this is generally good OO advice.

[BaerbelWinkler]

Thanks, Mike! I like a baby steps approach and will check it out tomorrow when I'm back in the office.

Cheers

Bärbel

[former_member620915]

I agree the most efficient way is to break the dependency, as suggested by Gábor Márián.

You can also use test seams/test injections to "fake" your productive code to have the data you want at your selects.

However...my main issue to your approach is that it is a little bit of brittle testing. Testing internals is NOT ok...having a test testing the privates of your class does not assure the behavior of your public interface.

You can only assert the behavior of a code through its public interface. Otherwise, someone can just remove the call to the private method, your test will still pass but the behavior of the class changed.

Testable design is the challenge (always), and TDD help with that. From what I saw of your private methods, you could test through the public interface with:
given some ddic
given source sys not ok for ddic

when <public method call>

then error message x should be returned.

[gabmarian]

The only point I am not agreeing with you is the use of test seams. The code under test should always be identical during productive use and unit testing. This construct inherently carries the possibility that something passing the tests can fail in normal use.

IMHO its introduction to the language was a big mistake.