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!
I'm glad we could help. Just a few comments:
"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 ?!)
It was a really interesting forum discussion.
My comment is
it is really more confortable & it helps you to follow Clean Code
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
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”
I like your blogs - they bring some real life into topics that sometimes seem bone dry...
keep it up,
Thanks for your feedback and encouragement, Michael!
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:
But personally I much prefer exceptions. Here you don’t even need to use functional methods.
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).
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:
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:
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.
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!
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( )
But I would go further and move the message table to be a private attribute of the class, e.g
and update all methods to reflect this. I think your method could then be written as:
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.
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:
move the logic to generate the exception to the class, i.e. in a method error_message( ).
Later you can merge the two in a new method.
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!
so the calling code will be simpler
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!
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.
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:
wrt to lo_check, I just used my preferred naming convention, YMMV.
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!
You actually got my point well. Sorry for the confusion.
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.
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:
New method to read the BAPIRET2-table:
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:
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!
I like how wb_action_allowed( ) can be written as a composition of functions:
I guess the exception can be avoided with
with v_dummy a string. The SYST structure would then be maintained.
Keep playing and best regards,
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:
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
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.
very nice read!
thanks so much for presenting your results and also pointing out the community spirit that helped you get there!
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
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.
The methods for checking the entries have a fairly similar “look and feel”:
And ENSURE_NO_ERROR_OCCURRED then just fills the “official” message fields if M_MESSAGE has content:
This article discusses the similarity between exceptions and go to. http://xahlee.info/comp/why_i_hate_exceptions.html
I used to program in Basic... back in the 80s. 😉
Thanks for the mention, and its a pleasure!