Skip to Content
Personal Insights

How SAP Community – and SAT – helped me to understand and write my first unit tests

Those of you who’ve been following my musings for a while are well aware that I do have my gripes with ABAP OO. In order to come to grips with that, I’ve started to dabble with some OO-coding for simple things like using local classes in report programs selecting some data and displaying it via ALV during the year. So, nothing really fancy but luckily simple enough for me to at least get my feet wet!

Discovering the beauty of functional methods

Recently, I had the need to create some checking logic (background story here) which I decided to do via a global class instead of a function module and to use Eclipse instead of SE80 to write the code. While developing the overall structure of the code based on some existing checking logic done in an old user-exit, I realised that I could break down the checks into small bits and pieces to actually create all of them as functional methods. The main method called from elsewhere ended up as just a string of nested IF-statements with each of them simply returning ABAP_TRUE or ABAP_FALSE. As the functional methods can also have exporting parameters, it was also possible to fill the message structure with some detailed information about the error encountered, if any. This then triggers the relevant error-message to be passed back to the calling routine.

As I knew that I would be struggling more than enough with ABAP OO itself, I didn’t do it the “right way” via TDD (test driven develompment) but made the concious and pragmatic decision to first write ABAP OO code I could understand and to then try and add unit tests later – if I could figure out how to do that, especially as I realised that I’d have to do some “mocking” in order to avoid actual table selects, something which seemed to be rather confusing to me.

After I had some working code which did (mostly) what I wanted it to do, I picked the brains of a colleague who knows more about ABAP OO than me and cleaned up the code some more as far as naming and breaking methods up into smaller methods was concerned. I also replaced fields from the SYST-structure within the called methods with import parameters as I had a hunch that e.g. sy-uname or sy-sysid would be problematic for the yet to be written unit tests.

  METHOD check_wb_action.
    DATA: message TYPE bapiret2.
    "Check objects in transport for DDIC-relevance
    IF ddic_objects_included( i_transport_objects ).
      "We have DDIC-objects in the transport - need to check system next
      IF system_for_ddic_allowed( EXPORTING  i_sysid   = i_sysid
                                  IMPORTING  e_message = message ).
        "We have DDIC objects in the transport and are in a system for DDIC-maintenance
        "Now need to check if user is allowed
        IF user_for_ddic_allowed( EXPORTING  i_uname   = sy-uname
                                  IMPORTING  e_message = message ).
          "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 
            APPEND message TO r_message.
          ENDIF.
        ELSE.
          "We have DDIC-objects in the transport and are in a system allowed for DDIC maintenance
          "but the user doesn't have the required authorisation. 
          APPEND message TO r_message.
        ENDIF.
      ELSE.
        "We have DDIC-objects in the transport but are not in a system allowed for DDIC maintenance
        APPEND message TO r_message.
      ENDIF.
    ELSE.
      "Now check that source system of all other objects in transport fits current system
      IF source_system_okay_for_object( EXPORTING i_transport_objects = i_transport_objects
                                                  i_sysid             = i_sysid
                                        CHANGING  e_message = message ).
        "All good and transport can be saved with objects
      ELSE.
        "At least one object in transport has another source-system
        APPEND message TO r_message.
      ENDIF.
    ENDIF.
  ENDMETHOD.

While working on the code, I become a bit of a fan of functional methods – and hope that I’ll find more reasons to actually make good use of them!

Adding unit tests to the mix

The next step was to try and wrap my head around this unit testing thing and how to deal with things like selects from tables as I had heard and read quite a bit about that but never fully grasped what all needs to fall in place to make it work. Terms like “mocking”, “interface” and “test doubles” were all a bit too abstract for my liking. So, before getting too confused by just reading up on unit tests, I decided to pick the brains of SAP Community – and am I ever happy that I did post Trying to add ABAP unit tests to a global class but could do with some help!

It didn’t take long for the first responses to get posted and as of this writing – 6 days after getting it out there – 10 answers and around 40 comments had accumulated. The longest back-and-forth had about 25 comments alone, with especially Matthew Billingham and Frederic Girod patiently responding to my seemingly never ending questions in order to clarify various items for me. Several others chimed in as well and even though I mostly used Matt’s suggestions, the other answers and comments also helped to improve my understanding. So, thanks must also go to Gábor Márián, Jacques Nomssi Nzali, Mike Pokraka, Paul Hardy, Felipe De Almeida Silva! This helpfulness is one of the big assets we have in SAP Community and I’m sure that I wouldn’t have the code in the shape it is now without it.

To cut a long story short – the nitty gritty details are in the Q&A thread mentioned above – here is just the list of the 16 unit tests I managed to write today and have all of them run successfully:

    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

I sure hope that they won’t be the only unit tests I’ll ever write!

Update: While poking around the unit test options in Eclipse a bit more, I discovered the coverage analyzer (CTRL+SHIFT+F11). After executing it, I tweaked some of the existing unit tests and added another one to get the overall coverage to 98.79%. Now, this is a really neat feature of unit tests!

SAT to the rescue!

Throughout this OO & unit testing adventure I had the feeling that I was missing something, like a “map” of how the actual and the test code interact with each other in order to know where I was and what was executed in which sequence. I tried to find out via debugging but that was just adding to my confusion – esp. as I tried to do it in Eclipse which turned out to be one new thing too many for me. Mulling this over some more and really wanting to peer under the hood, I turned to one of my favorite troubleshooting transactions: SAT.

In order to find out if SAT was up to the task, 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 via Eclipse and actually 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, it’s possible to see if some actual table selects have been overlooked and still require another set of “mock logic” for a table as shown in this view from an interim stage of things when I still needed to mock access to it:

So, what seemed like a daunting task to me at first, turned into a neat learning exercise, thanks to a great combination of community and tool help. The real test will obviously come when I try to create more ABAP OO code with unit tests and I wouldn’t be too suprised if there’ll then be another Q&A  thread from me asking for help!

29 Comments
You must be Logged on to comment or reply to a post.
  • I’m glad we could help. Just a few comments:

    • TDD is a way of programming, but not the right way.
    • I would use INSERT … INTO TABLE … rather than APPEND. It’s more robust as it works with tables with hashed and sorted keys
    • I prefer, still, to use the SAPGui debugger. It’s easier.
    • Thanks, Matt!

      “TDD is a way of programming, but not the right way.”

      Which is why I put “right way” in scare quotes (and yes, I still find it somewhat scary 😯!)

      Cheers

      Bärbel

  • It was a really interesting forum discussion.

     

    My comment is

    • Use Importing (could be several) & returning (only one !) in method definition

     

    it is really more confortable & it helps you to follow Clean Code

          IF system_for_ddic_allowed( EXPORTING  i_sysid   = i_sysid
                                      IMPORTING  e_message = message ).

    will be

          message = system_for_ddic_allowed( i_sysid ).

     

     

    but I think you have a returning with abap_bool also, so it will not directly work

    in clean code it could be something like

          IF is_message_a_success( system_for_ddic_allowed( i_sysid ) ).
    
    or
    
      data(message_ddic_allowed) = system_for_ddic_allowed( i_sysid ).
      if is_message_a_success( message_ddic_allowed ). 
    • Thanks, Fred!

      As I have the same basic logic for each of the checks with one or more importing parameters, the message structure as exporting and an abap_bool returning paramater, I’m inclined to leave it like it is. Otherwise the (to me) straightforward check logic would get more involved and couldn’t quite as easily get nested as it is now. Or, to use a saying “I want to have my cake and eat it too” 🙂

      Cheers

      Bärbel

  • Nice one, glad you’re ‘joining the dots’ 🙂

    I’m with Frederic that the use of both returning and importing is not ideal. I think there’s even some info on it in the ABAP help; and personally I do find it confusing.

    Other alternatives are to use a log/message object:

    data(messages) type ref to zcl_messages.
    if user_is_authorised( i_objects = objects 
                           i_messages = messages ).
    ...
    append messages->get_messages( ) to r_messages.
    ...
    
    ------------
    METHOD user_is_authorised. 
      IF  ... 
      ...
      ELSE.
        i_messages->add( 'No can do!' ). 
      ENDIF.
    ENDMETHOD.

    But personally I much prefer exceptions. Here you don’t even need to use functional methods.

    class lcx_error definition inheriting from cx_static_check.  "Or use a global exception class
    endclass. 
    
    ...
    TRY. 
        check_objects_are_allowed( ).
        check_user_is_authorized( ).
        check_source_system_is_valid( ). 
      CATCH lcx_error into data(error). 
        r_message = error->get_text( ).
    ENDTRY. 
    ...
    
    ------
    METHOD check_user_is_authorized. 
      IF.... 
       ... 
      ELSE.
        RAISE EXCEPTION TYPE lcx_error
          message e123(zblah) WITH sy-uname sy-sysid. 
      ENDIF.
    ENDMETHOD.

     

    • Thanks, Mike!

      I haven’t yet “progressed” to really understand and use exceptions, so I’m hesitant to rewrite my code to do that right now. I also don’t want exceptions to accidentally reach the calling routines as they may not deal with them properly (both just raise “cancelled” for their respective callers while filling the message structure).

      Cheers

      Bärbel

      • Exceptions are a world of controversy. Some claim they’re not much different from GOTO and so are bad. Others say don’t use them to handle conditions that mgiht not be right, but you can anticipate.

        There’s a trade of between

        check_situation – which throws an exception

        if_situation_has_happened( ) – an abap_bool returning functional method.

        One possibility is to keep the functional boolean method, and if it fails, query a variable. So something like:

        IF checker->it_is_all_fine_and_dandy( ).
          " Yeah - let's get on and do some work
        ELSE.
          write_out_message( checker->messages ).
        ENDIF.

        Where messages is a public read-only attribute of checker.

      • You should definitely play with class based exceptions a bit if you’ll have some spare time. 🙂

        There are multiple aspects making them superior compared to the “classic” ones. Just to name a few:

        • Propagation: if you e.g. work with a function module raising a classic exception you have to directly handle the error situation in the caller. Class-based exceptions on the other hand can travel up in the call stack until a suitable handler is found.
        • As the exception is an object itself additional information can be provided for the handler in form of attributes and methods.
        • You can further control the behavior via choosing the right base class of your exceptions. If you opt for a static check the compiler will help you to make sure no error is “leaking out” (via looking for an appropriate handler and giving you a warning if it is not found).

        I usually choose exceptions to handle events so serious that there is no point to continue if they occur. If I want to “take a note” and go on with the processing I use a logger class that collects the messages.

         

  • I was kinda hoping the linear version without all the nesting might convince you to give it a go 🙂

    Eclipse will warn you if you’re not handling an exception, and checking code in SE80 should also provide a warning.

    It really makes life so much easier than pushing messages around because you’re not dependent on the developer to remember to propagate messages.

    • Hi Mike,

      for at least one or two of the checks I need the ELSE-option to then check something else. It’s therefore most likely not going to be a  linear version anyway.

      Given the rather specific nature of this development, there won’t be (m)any other developers than myself looking at or tackling the code (it’ll be maintained in a central system not many have access to). And compared to the existing checks, this new class is already a lot cleaner – not to mention more robust due to the unit tests – than what it’ll (hopefully) replace!

      Cheers

      Bärbel

  • Hello Bärbel,

    now that you have are in the 90%+ of coverage, you can refactor safely. A conservative refactoring would be to add a return parameter rv_flag to method check_wb_action( )

     METHOD check_wb_action.
        DATA message TYPE bapiret2.
        
        rv_flag = abap_false.
        
        IF ddic_objects_included( i_transport_objects ).   " Check objects in transport for DDIC-relevance
                
          rv_flag = xsdbool( 
             system_for_ddic_allowed( EXPORTING  i_sysid   = i_sysid    " We have DDIC-objects in the transport - need to check system next
                                      IMPORTING  e_message = message )
            AND user_for_ddic_allowed( EXPORTING  i_uname   = sy-uname  " We have DDIC objects in the transport and are in a system for DDIC-maintenance        
                                       IMPORTING  e_message = message ) " Now need to check if user is allowed 
     
            AND source_system_okay_for_ddic( EXPORTING i_transport_objects = i_transport_objects "Now check that DDIC-system is source system of all objects in transport via TADIR
                                                       i_sysid             = i_sysid
                                             IMPORTING e_message = message ) ).
          ENDIF.
        ELSE.
          "Now check that source system of all other objects in transport fits current system
          rv_flag = source_system_okay_for_object( EXPORTING i_transport_objects = i_transport_objects
                                                             i_sysid             = i_sysid
                                                   CHANGING  e_message = message ).
        ENDIF.
        
        IF rv_flag = abap_false.
          APPEND message TO r_message.    
        ENDIF.
      ENDMETHOD.
    

    But I would go further and move the message table to be a private attribute of the class, e.g

    DATA mt_message TYPE bapiret2_t.

    and update all methods to reflect this. I think your method could then be written as:

     METHOD check_wb_action.
        rv_flag = abap_false.
        
        IF ddic_objects_included( i_transport_objects ).   " Do we have DDIC-objects in the transport ?
    
          rv_flag = xsdbool( 
                system_for_ddic_allowed( i_sysid )   " are we in a system for DDIC-maintenance?
            AND user_for_ddic_allowed( sy-uname )   
             " is DDIC-system source system of all objects in transport via TADIR ?
            AND source_system_okay_for_ddic( i_transport_objects = i_transport_objects
                                             i_sysid             = i_sysid ) ).
          ENDIF.
          
        ELSE.  "Now check that source system of all other objects in transport fits current system
          rv_flag = source_system_okay_for_object( i_transport_objects = i_transport_objects
                                                   i_sysid             = i_sysid  ).
        ENDIF.
      ENDMETHOD.

    With this, the code looks a lot more like a (functional) specification. Next I will be asking why system_for_ddic_allowed( ) is not hidden in source_system_ok_for_ddic() because, now that I can read the specification and think about it now, it seems to belong there. And so I would go on, try to refactor to  express my understanding of the specification in the code.

    As I said earlier, I would prefer more objects like user, tranport, ddic_ojbects. So the code would look like actors exchanging messages.

    I like seeing the sequence of events from SAT too. You can convert those into a sequence diagram.

    best regards,

    JNN

     

    • Thanks, Jacques for your suggestions to improve the code!

      I managed to get coverage up to 100% since adding the update and screenshot 🙂

      I actually (think I) need the message table to provide the error message back to the calling routine, so that it gets displayed in Eclipse. Not sure how I’d be able to ensure that if I only us a flag as a return parameter.

      This is how CHECK_WB_ACTION currently gets called from two different places:

          "Call the public method with a returning parameter as a BAPIRET2-table
          DATA(lt_check_result) = lcl_check->check_wb_action( EXPORTING i_transport_objects = objects
                                                                        i_sysid             = sy-sysid
                                                                        i_uname             = sy-uname ).
      
          "Read first (and only) entry in table and trigger message if one is found
          READ TABLE lt_check_result INTO DATA(ls_check_result) INDEX 1.
          IF sy-subrc EQ 0 AND
             ls_check_result-id IS NOT INITIAL.
            MESSAGE ID ls_check_result-id TYPE ls_check_result-type NUMBER ls_check_result-number
                  WITH ls_check_result-message_v1 ls_check_result-message_v2
                       ls_check_result-message_v3 ls_check_result-message_v4
               RAISING cancelled.
          ENDIF.

      Cheers

      Bärbel

      • Hello Bärbel,

        move the logic to generate the exception to the class, i.e. in a method error_message( ).

        IF NOT lo_check->check_wb_action( i_transport_objects = objects
                                          i_sysid             = sy-sysid
                                          i_uname  ).
          IF lo_check->error_message( ).
            RAISE CANCELLED.
          ENDIF.
        ENDIF.

         

        Later you can merge the two in a new method.

        regards,

        JNN

        • Hi Jacques,

          not really sure what you mean by “move the logic to generate the exception to the class

          Which “class” do you mean?

          I also don’t get “Later you can merge the two in a new method.

          Still more question than exclamation marks for me, I’m afraid!

          Cheers

          Bärbel

          • Hi Bärbel,

            1. my mistake: I meant to a new method error_message( ) in the same class.
            2. you can create a new method that encapsulates the details
            METHOD check_wb_action_new.
            rv_flag = abap_true.
            IF NOT check_wb_action( i_transport_objects = objects
                                              i_sysid             = sy-sysid
                                              i_uname  ).
              IF error_message( ).
                rv_flag = abap_false.
              ENDIF.
            ENDIF.
            ENDMETHOD.

            so the calling code will be simpler

            IF NOT lo_check->check_wb_action_new( i_transport_objects = objects
                                                  i_sysid             = sy-sysid
                                                  i_uname  ).
                RAISE CANCELLED.
            ENDIF.

            regards,

            JNN

          • Not sure if our definitions for “simpler” are the same! 😀

            You are basically suggesting to create a public “wrapper-method” around the method CHECK_WB_ACTION I currently have and move that from being public to private, right?

            To me it looks more complicated especially as I still don’t quite get what the new method ERROR_MESSAGE would contain and what else I’d need to change within global class ZCL_CHECK_WB_ACTION to keep things working as they are now.

            Oh, and what you show as “lo_check” is what I have – and probably named wrong – as “lcl_check”?

            Another stumbling block for changes is, that I have to be quite careful to not shoot myself in the foot due to the central location the class is used in: during activation of workbench objects. So, for example if I want to add an importing parameter to CHECK_WB_ACTION (as I did the other day by adding i_uname) I first have to do it as an optional parameter. If I try to add it as a mandatory parameter right away, I won’t be able to activate the calling routine’s code due to a syntax error – a nice little catch-22!

            Cheers

            Bärbel

          • Hello Bärbel,

            with a good unit test coverage, you can benefit refactoring, a workflow where you avoid make big changes that would break your code or your tests. It means to get used to some mechanical steps to make large structural changes to your code safely, in small tests (klein, klein).

            I have decided that the part where you trigger the message could be moved to a new method that is only used by the CHECK_WB_ACTION( ) method.This block should be moved to an error_message( ) method. 

             "Read first (and only) entry in table and trigger message if one is found
                READ TABLE lt_check_result INTO DATA(ls_check_result) INDEX 1.
                IF sy-subrc EQ 0 AND
                   ls_check_result-id IS NOT INITIAL.
                  MESSAGE ID ls_check_result-id TYPE ls_check_result-type NUMBER ls_check_result-number
                        WITH ls_check_result-message_v1 ls_check_result-message_v2
                             ls_check_result-message_v3 ls_check_result-message_v4
                     RAISING cancelled.
                ENDIF.

            But the RAISING clause shall be removed and a flag returned (functional method), so that we can trigger the exception in the caller code.

            This is a very common refactoring: Extract Method.

            My goal is actually to Tell. Don’t Ask: the caller should not need to know about the message table. So I will want to the signature of the CHECK_WB_ACTION( ) method to return a flag (OK, or Error). The message processing will be hidden in the method.

            To implement this safely, I proposed to create a new method just to follow the mechanics of the refactoring, e.g. Add Parameter:

            1. I create a new method with the new signature, that will just call the old method. I can create a unit test for the new method and make sure all tests pass.
            2. I can then replace calls of the old methods to the new method. After each step, I run the unit test to make sure they all pass.
            3. When the original method is not used anymore outside of the new, then I can move the logic and delete the old method. And make sure all tests pass.

            wrt to lo_check, I just used my preferred naming convention, YMMV.

            best regards,

            JNN

          • Hi Jacques,

            I’m still confused of how to do what you suggest.

            This paragraph has me confused:

            “My goal is actually to Tell. Don’t Ask: the caller should not need to know about the message table. So I will want to the signature of the CHECK_WB_ACTION( ) method to return a flag (OK, or Error). The message processing will be hidden in the method.”

            Wouldn’t his need to be CHECK_WB_ACTION_NEW in your example? And, shouldn’t this really have a completely different name anyway like e.g. WB_ACTION_ALLOWED (or something along those lines where checking a flag for “yes” or “no” than makes sense?

            And, if this new and public “wrapper” method then calls CHECK_WB_ACTION wouldn’t its signature need to change to no longer have the returning parameter in the form of a BAPIRET2-table? It would also need to use the import parameters i_transport_obects, i_sysid and i_uname passed in from the “wrapper” method, wouldn’t it? Using the sy-fields as you have in an earlier response would render the unit tests useless I think.

            In addition and as mentioned earlier, it’s not really easy to fiddle with the public methods called from the two calling places as there’s always the risk that changes to them can then not be activated. I don’t think that unit testing ZCL_CHECK_WB_ACTION helps with that issue a lot as internally to the class everything can work just fine but still cause issues from “outside”.

            As you can tell, I’m “somewhat” hesitant to change what I currently have working!

            Cheers

            Bärbel

          • Hi Bärbel,

            You actually got my point well. Sorry for the confusion.

            • yes this would be CHECK_WB_ACTION_NEW( ) in my example.
            • yes, it should be renamed to WB_ACTION_ALLOWED( ).
            • yes, it signature would not include the message table.
            • yes, it should include the import parameters to enable for unit testing.

            I admit that I did not consider activation. I remember having to set a break-point in production and to go trough the code step by step in the debugger once as a workaround to have it activated.

            On the other hand, your good test coverage works against fear, uncertainty and doubt: you try many code changes in small increments because it is easy to rollback the last changes or stop midway with working code.

            best regards,

            JNN

             

             

          • Thanks again, Jacques!

            So, I’ve played a bit more with my code:

            New public method to wrap CHECK_WB_ACTION w/o changing its signature yet so that it still returns the BAPIRET table:

              METHOD wb_action_allowed.
            
                r_result = abap_true.
            
                "Call the public method with a returning parameter as a BAPIRET2-table
                DATA(lt_check_result) = check_wb_action( EXPORTING i_transport_objects = i_transport_objects
                                                                   i_sysid             = i_sysid
                                                                   i_uname             = i_uname ).
            
                r_result = get_error_message( lt_check_result ).
            
              ENDMETHOD.

            New method to read the BAPIRET2-table:

              METHOD get_error_message.
            
                "Read first (and only) entry in table and trigger message if one is found
                READ TABLE it_check_result INTO DATA(ls_check_result) INDEX 1.
                IF sy-subrc EQ 0 AND
                   ls_check_result-id IS NOT INITIAL.
                  MESSAGE ID ls_check_result-id TYPE ls_check_result-type NUMBER ls_check_result-number
                        WITH ls_check_result-message_v1 ls_check_result-message_v2
                             ls_check_result-message_v3 ls_check_result-message_v4.
            *         RAISING cancel.
                  r_result = abap_false.
                ELSE.
                  r_result = abap_true.
                ENDIF.
            
              ENDMETHOD.

            This test-method for WB_ACTION_ALLOWED is now throwing error CX_AUNIT_UNCAUGHT_MESSAGE when an error-message gets set because the BAPIRET2-table contains an entry as expected. I unsuccessfully searched for an explanation or example so don’t know how to avoid this:

              METHOD wb_action_not_okay.
                "Table with objects includes DDIC-objects but workbench action is not allowed
                DATA(r_result) = cut->wb_action_allowed( i_transport_objects = VALUE #( ( object = 'PROG' obj_name = 'ZPROG' )
                                                                                        ( object = 'STRU' obj_name = 'ZSTRU' ) )
                                                         i_sysid             = 'DEV'
                                                         i_uname             = 'USER1' ).
                cl_abap_unit_assert=>assert_equals( act = r_result
                                                    exp = abap_false ).
              ENDMETHOD.
            

            I’ll need to set an error message internally to pass the relevant information back to the caller even if just indirectly by setting the msg-fields in the SY-structure. It would be kind of unfair to just tell the actual user that an error occured w/o mentioning which one it was!

            Cheers

            Bärbel

          • Hello Bärbel,

            I like how wb_action_allowed( ) can be written as a composition of functions:

              METHOD wb_action_allowed.
                r_result = get_error_message( check_wb_action( i_transport_objects = i_transport_objects
                                                               i_sysid             = i_sysid
                                                               i_uname             = i_uname ) ).
              ENDMETHOD.

            I guess the exception can be avoided with

            MESSAGE … INTO lv_dummy

            with v_dummy a string. The SYST structure would then be maintained.

            Keep playing and best regards,

            JNN

          • Happy New Year, Jacques!

            Now that I’m back in the office I changed the code to put the message into LV_DUMMY as you suggested and no longer get the error during unit testing.

            I also changed the two calling routines to the new public method:

                "Call the public method to find out if action is allowed
                IF lcl_check->wb_action_allowed( EXPORTING i_transport_objects = objects
                                                           i_sysid             = sy-sysid
                                                           i_uname             = sy-uname  ).
                  "Everything is fine
                ELSE.
                  RAISE cancel.
                ENDIF.

            And it works with the expected error message getting displayed for the user both when called via Eclipse and SE11 in the GUI.

            Now going to clean up the code some more, which may prompt more questions!

            Thanks much and Cheers

            Bärbel

          • And new questions – well, one – it did prompt!

            But, by now, I think I have the prototyped code in good enough shape to copy it into the productive dev-system.

            Cheers

            Bärbel

  • Hey Bärbel,

    very nice read!
    thanks so much for presenting your results and also pointing out the community spirit that helped you get there!

    Best
    Joachim

  • I have with everybody who says to use an exception rather than have an EXPORTING method to return any error messages.

    Functional methods should not really have EXPORTING parameters. A lot of people were horrified at the very idea.

    Robert Martin says that methods should do “one thing” and error handling is “one thing”. In this case you main method clearly has a RETURNING parameters which is a table of messages. In addition all of the contained methods pass back an error message as well.

    If all the lower level methods raised an exception which had the error message contained within it, then not only would you avoid the need to APPEND the R_MESSAGE lots of times, making the code a lot shorter (as shown in examples above) but the exception would bounce right out of the main method which now does not need a RETURNING parameter as it itself will catch and propagate the exception( if you define it to RAISE that exception).

    Then whatever method calls your main method can decide what do with the message table contained within the exception object e.g. show it to the user, write it to the application log, send an email or whatever. Thus the “one thing” of checking the transport has been separated from the “one thing” of dealing with any errors.

    You are worried about not knowing where the exception will end up, but if you make it STATIC then anything calling a method that raises that exception will have to deal with it – either handle it itself or pass it on, and then the method it passes it on to has the same situation to deal with.

    Matthew Billingham – appropros exceptions being like GOTO statements. When I used to program in BASIC you had to nominate the line a GOTO statement went to. With NO_CHECK exceptions where the program jumps to after the exception could be absolutely anywhere (or indeed nowhere) and the exact place could keep changing as the program evolves which is sort of the opposite of a BASIC GOTO – instead of saying GOTO LINE 10 you are saying GOTO SOMEWHERE I DON’T KNOW WHERE

    Cheersy Cheers

    Paul

    • Thanks for chiming in, Paul!

      My code has evolved quite a bit from what is shown in the snippets above and all methods are now truly (?) functional in that they only receive the necessary import parameters and have an ABAP_BOOL returning parameter. They no longer have/need an exporting parameter.

      Thus far, I’ve however still shied away from introducing proper exception handling and instead make do with a member variable to hold the error message. I don’t really need a table (and got rid of it just now) as the first error found will “seal the deal” anyway and stop the process with raising condition CANCEL in the calling routine.

      CLASS zcl_check_wb_action DEFINITION PUBLIC FINAL CREATE PUBLIC .
        PUBLIC SECTION.
      
          CLASS-METHODS:
            class_constructor.
      
          METHODS: wb_action_allowed
            IMPORTING i_transport_objects TYPE tr_objects
                      i_sysid             TYPE sy-sysid
                      i_uname             TYPE sy-uname
            RETURNING VALUE(r_result)     TYPE abap_bool.
          [....]
      
        PRIVATE SECTION.
         [....]
      
         METHODS source_system_okay_for_object
            IMPORTING
              i_transport_objects            TYPE tr_objects
              i_sysid                        TYPE sy-sysid
            RETURNING
              VALUE(r_result) TYPE abap_bool.
      
          METHODS ensure_no_error_occurred
            RETURNING
              VALUE(r_result) TYPE abap_bool.
      
          "Member variable to hold error message
          DATA: m_message                   TYPE bapiret2.

       

      The methods for checking the entries have a fairly similar “look and feel”:

        METHOD source_system_okay_for_object.
          "Default assumption is that object-maintenance happens in expected source system
          r_result = abap_true.
          CLEAR m_message.
      
          LOOP AT i_transport_objects INTO DATA(transport_object).
            DATA(lv_srcsystem) = _dao->get_srcsystem_from_tadir( transport_object ).
            IF lv_srcsystem NE space.
              "Entry found - check source system
              IF lv_srcsystem NE i_sysid
                "Not okay as source system differs from actual system
                r_result = abap_false.
                m_message-type        = 'E'.
                m_message-id          = 'ZBASIS'.  ##TODO-new-MSGClass
                m_message-number      = '001'.     ##TODO-new-MSGNumber
                m_message-message_v1  =  lv_srcsystem.
      
                "First error triggers message and return to caller
                EXIT.
              ENDIF.
            ELSE.
              "New object, no problem
            ENDIF.
          ENDLOOP.
        ENDMETHOD.

       

      And ENSURE_NO_ERROR_OCCURRED then just fills the “official” message fields if M_MESSAGE has content:

        METHOD ensure_no_error_occurred.
          DATA: lv_dummy TYPE string.
      
          IF m_message-ID     IS NOT INITIAL AND
             m_message-number IS NOT INITIAL.
            MESSAGE ID m_message-id TYPE m_message-type NUMBER m_message-number
                  WITH m_message-message_v1 m_message-message_v2
                       m_message-message_v3 m_message-message_v4
                  INTO lv_dummy.
            r_result = abap_false.
          ELSE.
            r_result = abap_true.
          ENDIF.
      
        ENDMETHOD.

      Cheers

      Bärbel