ABAP Unit Test meets Legacy Code
All Blogs in the series
1. ABAP Unit Test Week 1 |
2. ABAP Unit Test in Odata |
3. ABAP Unit Test TDD Implementation |
4. ABAP Unit Test Meets Legacy Code. |
Finally done with week 3 of Open SAP course where we saw how to add ABAP unit testing in existing legacy code. We already had a great feedback back about Week 3 in this blog by Paul. I thought of applying it in one of near to actual scenario to see how it looks. I have few observations on pair programming which hopefully I will share via separate blog.
Background of Existing Report
This is a 5+ years old report written in a procedural way with all the global data declarations etc. What it does is as follow
- Check for a user whose name begins with XXX* and valid to date is greater than current date
- If valid user check number of days between user creation and valid to date, if greater than 60 it modifies the validity date to creation date + 60 days
Awesome Features of current legacy report:)
- No selection screen
- 60 days is hardcoded.
- Username pattern is hard coded
Sample code to change the validity
* Difference between from and to days
CALL FUNCTION 'HR_HK_DIFF_BT_2_DATES'
EXPORTING
date1 = dateto
date2 = datefrom
output_format = '02' " Out put in days
IMPORTING
days = dayscount
EXCEPTIONS
invalid_dates_specified = 1
OTHERS = 2.
IF sy-subrc <> 0.
MESSAGE ID sy-msgid TYPE sy-msgty NUMBER sy-msgno
WITH sy-msgv1 sy-msgv2 sy-msgv3 sy-msgv4.
ENDIF.
IF dayscount GT c_limit.
newdate = gw_logond-gltgv + 60 .
MOVE-CORRESPONDING gw_logond TO datenew.
datenew-gltgb = newdate.
* Change the Valid Through date , in case of days of validity is more than 60
CALL FUNCTION 'BAPI_USER_CHANGE'
EXPORTING
username = usr02-bname
logondata = datenew
logondatax = change
TABLES
return = return.
IF sy-subrc IS INITIAL.
CALL FUNCTION 'BAPI_TRANSACTION_COMMIT'.
ENDIF.
ENDIF.
ENDIF.
Enhancement Needed.
This concept now needs to be extended to another set of users whose name begin with YYY* and whose validity shall be for 45 days. Easiest and the worst solution will be to add mine YYY* user id and hardcode another 45 days with IF and ELSE stuff. But I thought of why don’t we implement it via unit test class, it will make every life easier, testable and flexible.
Implementing Unit testing in Legacy code
The most important place in the code is where it decides whether the user needs to be modified or not that is where all action will happen. So started by creating our local test class with method ABC with parameters difference between number of days and username.
So started by creating local unit test class LTC_USERVALIDITY with method test method USER_DAYS for testing. This method will pass Username, Number of days between creation and valid to date and creation date to determine whether a user needs to be updated or not. If update is required Y flag is returned along with calculated date.
CLASS ltc_uservalidity DEFINITION FOR TESTING
RISK LEVEL HARMLESS
DURATION SHORT.
PRIVATE SECTION.
DATA: m_cut TYPE REF TO lcl_uservalidity.
METHODS setup.
METHODS user_days FOR TESTING RAISING cx_static_check.
ENDCLASS.
CLASS ltc_uservalidity IMPLEMENTATION.
METHOD setup.
m_cut = NEW lcl_uservalidity( ).
ENDMETHOD.
METHOD user_days.
" 60 Days test all possible combinations for XXX
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'XXX123'
days = 60
startdate ='20180130'
IMPORTING r_result = DATA(checkvalue) r_newenddate = DATA(enddate) ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'Y' ).
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'XXX123'
days = 59
startdate ='20180131'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'N' ).
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'XXX123'
days = 61
startdate ='20180129'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'Y' ).
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'XTZ123'
days = 59
startdate ='20180131'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'N' ).
" 45 Days test all possible combinations for YYY
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'YYY123'
days = 45
startdate ='20180130'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'Y' ).
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'YYY123'
days = 44
startdate ='20180130'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'N' ).
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'YYY123'
days = 48
startdate ='20180130'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'Y' ).
m_cut->check_userendate_upd_needed( EXPORTING
userid = 'XTZ123'
days = 49
startdate ='20180130'
IMPORTING r_result = checkvalue r_newenddate = enddate ).
cl_abap_unit_assert=>assert_equals( act = checkvalue
exp = 'N' ).
ENDMETHOD.
ENDCLASS.
A custom table for mapping username with number of days was also created.
Then defined the actual method CHECK_USERENDATE_UPD_NEEDED to check from custom table whether a user needs to be updated or not.
CLASS lcl_uservalidity DEFINITION
FINAL
CREATE PUBLIC .
PUBLIC SECTION.
DATA: username TYPE bname,
days TYPE i,
userdays TYPE STANDARD TABLE OF zuserdays.
METHODS constructor.
METHODS check_userendate_upd_needed
IMPORTING
userid TYPE xubname
days TYPE pea_scrdd
startdate TYPE sy-datum
EXPORTING
r_result TYPE string
r_newenddate TYPE sy-datum .
PROTECTED SECTION.
PRIVATE SECTION.
ENDCLASS.
CLASS lcl_uservalidity IMPLEMENTATION.
METHOD constructor.
" Master data for days for user name
SELECT * FROM zuserdays INTO TABLE userdays.
ENDMETHOD.
METHOD check_userendate_upd_needed.
DATA:ls_userdays TYPE zuserdays,
uname TYPE bname.
uname = userid+0(3).
READ TABLE userdays INTO ls_userdays WITH KEY username = uname .
IF sy-subrc EQ 0.
IF ls_userdays-zdays > days.
r_result = 'N'.
ELSE.
r_result = 'Y'.
r_newenddate = startdate + ls_userdays-zdays.
ENDIF.
ELSE.
r_result = 'N'.
ENDIF.
ENDMETHOD.
ENDCLASS.
Added the selection screen parameter for user type
Finally removed the hardcoded part added our user date extender determiner plugin:)
* Hardcoded part commented added our plugin which determine if date needs to
* to be changed or not if yes then calculate also.
* IF gv_count GT c_limit.
* gv_newdate = gw_logond-gltgv + 60 .
uservalidity->check_userendate_upd_needed( EXPORTING
userid = gw_bname-bname
days = gv_count
startdate = gv_fromdate
IMPORTING r_result = DATA(checkvalue) r_newenddate = DATA(enddate) ).
IF checkvalue = 'Y'.
MOVE-CORRESPONDING gw_logond TO gw_newdate.
gw_newdate-gltgb = enddate.
* gw_newdate-gltgb = gV_newdate.
It worked flawlessly.
Benefits
- Clearly any change in report done does not matter by whom he/she shall be able to validate that at least this part of the code is not impacted by his/her change.
- Once the ABAP unit framework is set normally people tend to follow it, so hopefully this program will eventually become better with each iteration.
- I came to know about the concept of Test Seam introduced in latest ABAP version, for more detail read this blog by Klaus Ziegler
Challenges or Open Points
- One of the important thing which I am still thinking is how do I add flexibility to my code not only in terms of username and days, but also if a new criteria needs to be added later for example user type? Some kind of a similar abstraction concept needs to be bring in, will share as soon as I have got it implemented.
- Reluctance to write so much code for just another if and else
What next from here?
- Some thing about Test Seam
- ABAP Test Double framework.
Feel free to provide your feedback open to all ears.
Just curious, why did you put all the tests into one method? If one fails, you wouldn' necessarily know which one.
Also, your new class is a local class to the report? Where does the test class go? A separate include?
Thanks for the example.
Justin
Hello
Thanks for the feedback.
Test class goes as local include. Logically created one method as they are related tests, we can create different methods for logically different criterias. The ouput when you run shows the line number where it failed.
Thanks
Nabheet
Thank you Nabheet.
Hi,
firstly nice that you took the effort to make your own example and to write about it!
Regarding the test method, I thought the same like Justin. Why didn't you create separate methods? Don't get me wrong, I really appreciate that you went the extra mile, therefore I want to provide helpful feedback (even though it might seem picky...).
It would be nice to see your tests go wrong (red) once and what you changed about it, also a screenshot of all tests being green would be nice...just to put some more attention on the ATC part and to show your experiences with it.
Just for the bigger picture, would it we possible to share the complete report with test class?
Cheers,
Mark
Thanks Mark for the feedback. Since kind of tests to be run is more or less same that is why. OOPs let me add screen shots sorry missed it...
Hello Nabheet,
thanks for sharing.
Actually I have some old test methods that looks similar to yours user_days method, but
as told in open sap wtc course, it should be easier to write a new test (a new test variant) compared to existing ones. With given test method it would be hard to do that as this test method would grow with each new test variant.
From my point of view, there are two methods to improve your/our test code.
First one would be to follow advices from Justin Loranger and Mark Wagener and create different test methods (and even different test classes).
In your example that could be like this:
ltc_user_xxx_upd_needed_cause (test class)
– days_eq_60 (test method)
– days_eq_61 (test method)
ltc_user_xxx_no_upd_needed_cause (test class)
– days_eq_59 (test method)
– user_name_eq_XTZ123 (test method)
ltc_user_yyy_upd_needed_cause (test class)
– days_eq_45 (test method)
-…
The other option would be to group similar test methods.
In your example that could be like:
ltc_user_validity_check_if (test class)
– update_is_needed (test method)
– no_update_is_needed (test method)
best regards,
Alex
I love reading comments like these. They make me remember why I read comments. Nice adds to the blog.
Michelle
Thanks Alexander for the great feedback. Grouping similar test methods logically is something i can relate to. This is the reason for sharing the experience to get 360 degree view.
Thanks once again
Hey Nabheet,
thanks for sharing!
On
> I am still thinking is how do I add flexibility to my code not only in terms of username and days, but also if a new criteria needs to be added later for example user type?
I would say: don't worry about possible future use-cases now - they might never come! If you get a new requirement, e.g. with the new criteria, then is the time to think about how to implement it!
best
Joachim
I'm with you here -> don't change the code (yet), but thinking is never a bad thing. Put your thoughts for you colleges (or for your future self) as comment in the code, for example "for more flexible checks -> use BRF+".
Cheers, Uwe
Thank you for a real world example!!!!! Nicely done.
Michelle
Thanks appreciate the feedback
Thanks for sharing.
Just two comments:
Since your business logic/decision is based on DB table:
1. Isn't using all these hard-coded checks w/o a test seam (or mockup data) quite useless?
(Although it seems you are going to talk about test seams on next part).
2. Is there any point in checking both XXX and YYY users values? (Currently there are only two records, but I guess you aren't going to cover any data record which would be added in the future?)
Thanks Shai for the feedback. Please find below my response.
Thanks
Nabheet
Hi,
Can i get more examples on how to write unit tests for function modules
Regards,
Ramana.