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

[hardyp180]

If my book seems to be suggesting that you need to make a method public in order to test it i.e. change its category for the sole purpose of making it testable, then I need to revisit that chapter. Making code testable does alter the design of a program (for the better) but in this case since the production code has no idea it can be tested the decision to make a method public or private should be made in the normal way (i.e. what other objects need to access that method) irrespective of whether you are going to test it or not.

As we have seen many people do not like testing private methods, but it can be done via that FRIENDS mechanism. I can see both sides of the argument but sometimes when a private method in my application is playing silly buggers then I start testing it to bits until I can get it to behave.

Whatever you do, do not use TEST SEAMS. They are an abomination, the production code should have no idea it can be tested. The effort/risk of changing the production code to include a test seam is the same (or more) as abstracting that bit of code to a method which can be mocked.

What does have to be public are the methods of your database access class, as those methods are defined by an interface.

In the Open SAP course the instructors suggested using "dependency lookup" whereby whenever you want an instance of your database access class you get it via a factory. In real life an actual class is returned, in tests a test double is "injected" to the factory, and the production code has no idea a test double is doing the work instead of a real class instance.

I had never heard of that idea, but it is genius, and my colleague and I implemented it at work the very next day.

Some people would say it is bonkers to have a unit test where all you do is invoke one method of a test double which returns a hard coded result. It does seem silly on the face of it. However to get to the stage where such a test works you have to improve the design of your program to a stage where that test is even possible, and testable code is better code, it is as simple as that.

Cheersy Cheers

Paul

[BaerbelWinkler]

bfeeb8ed7fa64a7d95efc21f74a8c135

Thanks for chiming in, Paul!

Your book in chapter 5 doesn't suggest to make methods public to make them testable. What it states as a side-note is along these lines: "[...] explains why it's quite okay to introduce FRIENDS in order to allow unit tests for private methods".

Cheers

Bärbel

[FredericGirod]

Hi Paul,

I really like your book, honestly this is for me a very good book, it is written by a developer for developers. But, the Abap Unit part is really difficult to understand when you begin in Unit Test. I know it is not a dedicated book on this subject, but I wonder if you scare a little bit some developers.

dealmf informs me there is a new dedicated book on TDD Test Driven Development With Abap Object

Fred

[BaerbelWinkler]

frdric.girod

Hi Fred,

thanks for the link to the new book about TDD!

I'm however not sure if a book with 594 pages isn't a lot more daunting than Paul's chapter with not quite 60 pages!

Cheers

Bärbel

[FredericGirod]

Abap to the future is 864 pages ! and it is really nice to read ! Size definitively does not means anything 😉

[nomssi]

Hello Bärbel,

I struggled to understand your class responsibilities.The name ZCL_CHECK_WB_ACTION suggests a function, not an object. It could be changed to ZCL_CHECK_WB_ACTION_MGR, but I tried to discover objects your description:

performing some checks before DDIC-objects get added to a transport

With this, I have 2 objects in your domain: [TRANSPORT] , [DDIC_Objects]. I would then aim at creating to methods

• Transport->Add( DDic_Objects ) and
• Transport->Check( DDic_Objects ) (could be renamed to Can_Add( ) ) returns a boolean.

IF those helps to write a simpler main program, then this abstraction is worth the effort of creating new classes and writing unit tests for them.

regards,

JNN

[BaerbelWinkler]

Hi Jacques,

thanks for your comment, which I must admit flies right over my head!

The class is mostly just checking whether the user is allowed to add DDIC-objects to a transport in the development system used (plus some rather specific checks we need to do).

The reason I'm doing this is related to this ongoing discussion:

Is there an alternative for Eclipse/ADT to workbench-related user exits like EXIT_SAPLSEDD_001?

I was contemplating wether or not to create it as a static class but the general recommendations I read seem to discourage that. I'm not really bothered about things like "class responsibilites" at the moment - I'm focused to keep things simple for me even if that means to not create the "perfect" ABAP OO code. I want to understand what I'm doing and "clean ABAP OO code" doesn't yet fit the bill for me.

Cheers

Bärbel

[nomssi]

Hi Bärbel,

to put my answer in perspective:

there nothing like perfect ABAP OO code. You start somewhere (first make it run) and make refinements (than make it right). My fear is, as your are not using TDD, you will have to delete all your unit test code and re-write it later after refactoring. It is a good thing if you learn something in the process, but it may makes you abandon unit testing altogether.

I tried to move the discussion from the implementation of a Data Access Object for mocking purposes to the general overview of the Objects/Classes needed. You have hindsight from your existing implementation so it is make it right time: focus on making the code the easy to maintain.

Your narrative presents a functional view of problem:

The class is ..checking whether the user is allowed to add DDIC-objects to a transport in the development system used.

Object oriented code always assigns responsibilities, conscientiously or not. If you have more than one class, by deciding where to implement a method you assign responsibility to fulfill a task to a given object.

I would extract logic from the current code into new objects/classes designed to represent abstractions of the problem domain. A process to discover objects is to read the description (what is this logic supposed to do) and map names to objects, and verbs to methods (e.g objects: user, transport, DDIC-object, methods add( ), check( )). Now we can imagine how the objects cooperate to achieve their tasks and define the method signature, e.g.

       Transport->Check( objects = DDic_Objects   
                         user = user )

This form of domain driven discovery of objects makes you code speak the same language as you your narrative. The objects are valuable for the code maintenance. I would write unit tests for those.

best regards,

JNN

[BaerbelWinkler]

Thanks, Jacques!

My problem with all of this is the tendency of "klein, klein" ("small, small") which makes me lose the big picture and consequently get lost in all the details and no longer knowing where I am. For me it's easier to have everything in one or two classes even if that is not "pure" ABAP OO. The methods I now have in the main class do simple checks which is the common denominator, the other class reads the DB-tables. Breaking this up further into separate classes by object would most likely lead to at least a handful of classes with one (check) method each which to me doesn't make much sense - at least in light of my ongoing struggles with ABAP OO in general.

Cheers

Bärbel

[BaerbelWinkler]

Here is an update as the thread is getting longer and longer thanks to the help I'm getting from matthew.billingham, gabmarian, frdric.girod, jacques.nomssi, mike.pokraka, bfeeb8ed7fa64a7d95efc21f74a8c135, dealmf!

I'm slowly getting closer to understanding what I need to do where (and why) and I think I found a neat way to help me with that: SAT. Anybody reading through the thread will notice, that I easily get confused on what gets actually executed while running unit tests. So, I wondered if SAT might be able to help me and in order to find out I set up a scheduled execution without aggregation and restrictions apart from specifying my global class ZCL_CHECK_WB_ACTION for the object name. I then executed the unit tests (4 at the moment) via Eclipse and did get what I think is a helpful result in SAT!

For one, I can now see the sequence of events for each of the defined unit tests and which routine gets called via the test double:

For another, I see that I'll have to create another set of "mock logic" for a Z-Table which still gets called while unit testing (this is of course not a surprise and I knew that I'll need to tackle this as well):

For proper unit testing, this tab in SAT will always need to be empty, I guess.

[matt]

Using SAT is a great idea. Yes - the DB Tables tab should be empty for most scenarios.

[BaerbelWinkler]

As the sub-thread with matthew.billingham and others' very helpful coaching was getting too long, I'm starting a new one with the current state of affairs as the startig point. I successfully managed to mock the 2nd table accessed (ZBC_DDIC_CHECK) by duplicating and then tweaking the logic to mock TADIR. The code of the resutling test-class is in the attached file code-testclass-prototype.txt

Instead of checking the issued error-message as Matt did in his example, I'm just checking if the message table includes entries with message-type = E":

  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.

My version looks like this:

  METHOD old_no_ddic_sys_not_ok.
    "Activity in correct system but source system of existing non DDIC-object okay
    DATA(messages) = cut->check_wb_action( i_transport_objects = VALUE #( ( object = 'INCL' obj_name = 'ZOLD_INCL' ) )
                                           i_sysid             = 'DEV1' ).
    cl_abap_unit_assert=>assert_not_initial( messages ).
    cl_abap_unit_assert=>assert_equals( act = messages[ 1 ]-type exp = 'E' ).
  ENDMETHOD.

At the moment the code under test has logic like this for the message handling:

IF _dao->entry_exists_zbc_ddic_check( EXPORTING i_check_title    = 'DDIC-OLD-SYSTEM'
                                                i_checked_entity = lv_srcsystem ).

  r_correct_source_system = abap_false.
  e_message-type        = 'E'.
  e_message-id          = 'ZBASIS'.
  e_message-number      = '002'.
  e_message-message_v1  =  lv_srcsystem.
  EXIT.
ENDIF.       

Which leads me to the question if error-messages can be "mocked" and if so how and if it's even worth the effort?

Checking the error message directly as done by Matt could cause unexpected results (I think) if the message gets assigned via a message-id and number (as I eventually plan to do for all "my" error messages) and therefore gets internally determined from table T100 and sy-langu. So, even if the correct message gets triggered, the test might still fail because it's checking the hard-coded English text against for example the determined version of the same message in German if that's the language the user running the tests is logged-in with.

A "compromise" might be to just look for message-type = 'E' as I now did even if that is rather unspecific. Perhaps the best (?) option is to check against message-ID and message-number for the tests as these can be tied to each error condition specifically.

Are there any best practices for this?

[FredericGirod]

Hi

For my point of view you have to test message Type, Id, Number. Not the text.

When I create a method, I start to put a comment "My responsability is ..... " and the corresponding Unit test have to test this responsability, nothing more, nothing less.

In your case, the responsability is to answer the good sentence? or the good message number ? or the message type? Depending on this answer you will know what to test.

Remember, Object oriented code is only a matter of responsability.

[matt]

I use either type, id number or text element. Not hardcoded text. That way there's no problem with translation.

The way I look at it, is that if you've got ONE abap unit test in the whole program, then that's better than no tests at all. Or to put it another way - always leave a program in a better state than when you found it.

As far as a plethora of classes - since (among many other things) I write commercial software, many classes is a good thing, as it makes it much easier to issue a fix when you only have to send a specialised component - even just a method - rather than the whole application as you might have to do if you have just one class. For writing code that's only used within your employer, it's just not so critical.

One tip. You don't have to have the DAO (in our example) as a global interface and class. You can make it local. It still works just as well - and has the added bonus (from your point of view) of keeping the code in one place and so easier to grasp.

[BaerbelWinkler]

Thank you all for helping me to better understand unit testing! Wish I could accpet more than one answer, but that unfortunately (still) isn't possible.

I now have 16 different unit tests available to ensure that I don't break anything while changing the code:

    METHODS:
      setup,
      ddic_check_active           FOR TESTING RAISING cx_static_check,
      ddic_check_from_se11_active FOR TESTING RAISING cx_static_check,
      called_from_eclipse         FOR TESTING RAISING cx_static_check,
      not_called_from_eclipse     FOR TESTING RAISING cx_static_check,
      ddic_system                 FOR TESTING RAISING cx_static_check,
      not_a_ddic_system           FOR TESTING RAISING cx_static_check,
      user_okay_for_ddic          FOR TESTING RAISING cx_static_check,
      user_not_okay_for_ddic      FOR TESTING RAISING cx_static_check,
      ddic_objects_included       FOR TESTING RAISING cx_static_check,
      no_ddic_objects_included    FOR TESTING RAISING cx_static_check,
      old_ddic_src_ok             FOR TESTING RAISING cx_static_check,
      old_ddic_src_not_ok         FOR TESTING RAISING cx_static_check,
      new_ddic_src_ok             FOR TESTING RAISING cx_static_check,
      old_ddic_sys_not_ok         FOR TESTING RAISING cx_static_check,
      old_no_ddic_sys_ok          FOR TESTING RAISING cx_static_check,
      old_no_ddic_sys_not_ok      FOR TESTING RAISING cx_static_check

And yes, naming of the test-methods could most likely be improved, but it's a start and a lot more than I had not even a week ago!

Thanks again & Cheers

Bärbel