Unit testing methods which are calling static methods

Arseni Mourzenko
Founder and lead developer
159
articles
July 19, 2019
Tags: rant 32 refactoring 11 unit-testing 5 design 2

Dis­claimer: the ar­ti­cle be­low talks about meth­ods call­ing pub­lic sta­t­ic meth­ods from oth­er class­es. This ex­cludes the calls to pri­vate sta­t­ic meth­ods.

Re­cent­ly, I wrote a rather un­pop­u­lar an­swer on Stack Ex­change about sta­t­ic meth­ods in a con­text of unit test­ing. The orig­i­nal ques­tion was about ways to test meth­ods which, in turn, rely on sta­t­ic meth­ods, and was it­self il­lus­trat­ed by a method which re­lies on a sta­t­ic ToHex which takes a byte ar­ray as a pa­ra­me­ter and re­turns a string which is a hexa­dec­i­mal rep­re­sen­ta­tion of the ar­gu­ment.

I an­swered that this is specif­i­cal­ly one of the prob­lems with sta­t­ic meth­ods. In­stead, I sug­gest­ed to use de­pen­den­cy in­jec­tion in or­der to be able to use dur­ing run­time an ac­tu­al con­vert­er from a byte ar­ray to a string con­tain­ing a hexa­dec­i­mal rep­re­sen­ta­tion, and dur­ing tests, a stub.

This ques­tion led to a dis­cus­sion with one of the mem­bers of Stack Ex­change com­mu­ni­ty who con­tact­ed me by e-mail. In my re­sponse, I ex­pand­ed on the sub­ject of unit tests, sta­t­ic method and de­pen­den­cy in­jec­tion. I be­lieve it would be use­ful to share my an­swer here, since the sub­ject ap­pears to be quite con­fus­ing for some peo­ple. I was, for in­stance, quite sur­prised to find the same sub­ject in the com­ments of my oth­er an­swer.

Call­ing sta­t­ic meth­ods from an in­stance method makes life mis­er­able. Es­pe­cial­ly when it comes to unit test­ing. This has ab­solute­ly noth­ing to do with state. The prob­lem is that the code un­der unit test re­lies on log­ic which is some­where else, out­side the test­ed method or even the class the method be­longs to. This cre­ates its own prob­lems:

  1. Unit tests are not unit any longer. You can't fo­cus on a spe­cif­ic class and its in­ter­face with oth­ers. In­stead, you need to con­sid­er the class it­self, but also all the sta­t­ic meth­ods it calls. And the sta­t­ic meth­ods called by the orig­i­nal sta­t­ic meth­ods. And so on. Some sta­t­ic meth­ods could be with­in li­braries and frame­works which don't be­long to you, which doesn't make things any eas­i­er.

  2. When a sta­t­ic method, used out­side its class, is changed, it should break its unit tests, but could also break unit tests of oth­er meth­ods which sim­ply re­lay on this sta­t­ic method. This is the op­po­site of what one wants from unit tests: what one wants is for those tests to pin­point ex­act­ly and un­am­bigu­ous­ly the lo­ca­tion of a re­gres­sion. If the build shows me that my last mod­i­fi­ca­tion broke six­ty tests, I have to spend time do­ing work that should have been done by the unit tests in the first place.

  3. When pro­ject grows, one of the com­plex­i­ties which may ap­pear is that a spe­cif­ic im­ple­men­ta­tion of some­thing is not enough. In­stead, there should be dif­fer­ent vari­ants for dif­fer­ent sit­u­a­tions.

Long ago, when I was us­ing C#, I was reg­u­lar­ly hit­ting an an­noy­ing pat­tern—well, reg­u­lar­ly, un­til I solved it. I start a pro­ject us­ing sta­t­ic meth­ods such as File.ReadAllText or File.Copy, and one day, I need to store data in iso­lat­ed stor­age. So I spend the next hours re­nam­ing all the meth­ods in or­der to use Isolated­Storage­File.Copy­File (that's not sta­t­ic, hey!) and sim­i­lar ones, and then I ob­vi­ous­ly for­get some of them, and the ap­pli­ca­tion doesn't work as ex­pect­ed, and I ask my­self what damn fool I was to think that I would have to rely on sta­t­ic meth­ods from File class for­ev­er. Lat­er on, I need to sup­port both iso­lat­ed stor­age and BLOB stor­age to a re­mote data­base, and I fi­nal­ly do what I could have done from the be­gin­ning: prop­er de­pen­den­cy in­jec­tion with an in­ter­face which con­tains the com­mon meth­ods to work with files, and which is ready to have as many im­ple­men­ta­tions as need­ed: one which sim­ply re­lies on File mem­bers un­der the hood, an­oth­er one which doesn't hit the stu­pid 259-char­ac­ters lim­it in path names, an­oth­er one which re­lies on a data­base, an­oth­er one which adds caching, etc.

It is sur­pris­ing to see how of­ten de­vel­op­ers as­sume that a bunch of things will nev­er change. Some be­lieve that it is per­fect­ly OK to as­sume that the pro­ject will al­ways rely on MySQL. Oth­ers think that it's fine to link their ap­pli­ca­tion to Share­Point, in­stead of treat­ing Share­Point as just a de­pen­den­cy, which it is. Oth­ers be­lieve that sta­t­ic meth­ods from .NET Frame­work are the way to go. The fact is, re­quire­ments change, and if your code is in­ter­linked with some­thing that you thought will be here for­ev­er, you'll suf­fer.

To make life slight­ly less mis­er­able, one needs to use de­pen­den­cy in­jec­tion. It its most el­e­men­tary form, the caller could be in­ject­ing an ac­tion to ex­e­cute at some point. More com­plex cas­es would re­quire to in­ject an ob­ject which im­ple­ments a spe­cif­ic in­ter­face which de­fines which ac­tion or ac­tions can be called.

A bit too ab­stract? Well, let's talk by giv­ing con­crete ex­am­ples.

Un­for­tu­nate­ly, peo­ple who are hap­py call­ing sta­t­ic meth­ods give re­al­ly bad ex­am­ples. For in­stance, they ask: “OK, let's say you have code which should add two num­bers. Would you in­ject an ob­ject which im­ple­ments an ad­di­tion op­er­a­tor, or would you sim­ply call a sta­t­ic method which does the damn ad­di­tion?” My an­swer is nei­ther. When I need to com­pute the sum of x and y, I don't use de­pen­den­cy in­jec­tion and I don't call sta­t­ic meth­ods. I do z = x + y, and it ends there. This ex­am­ple is dumb, so let me give more re­al­is­tic ones.

Ex­am­ple from the pro­ject 1

A sim­ple ap­pli­ca­tion works with some set of data. Some data is stored in bi­na­ry for­mat, en­closed with­in the Blob class to the re­main­ing part of the ap­pli­ca­tion. At some point, the BLOBs need to be dis­played in hexa­dec­i­mal for­mat. The con­ver­sion is need­ed strict­ly in a sin­gle lo­ca­tion (i.e. when­ev­er we have an in­stance of the Blob class, we could end up do­ing a con­ver­sion to hexa­dec­i­mal, but nev­er out­side the pres­ence of the Blob class).

While some pro­gram­mers could be in­clined to cre­ate a sta­t­ic “util­i­ty” class with the con­ver­sion to hexa­dec­i­mal in­side, I strong­ly ad­vise against this ap­proach (and I'm also strong­ly against util­i­ty class­es in gen­er­al). The con­ver­sion method clear­ly be­longs to the Blob class it­self. It would pos­si­bly end up as a pub­lic in­stance method ToHex, or, if too com­plex or used in­di­rect­ly by the BLOB it­self, it could also be a pri­vate sta­t­ic mem­ber of the class.

The place of the method, as well as its vis­i­bil­i­ty, is es­sen­tial here when it comes to test­ing. The con­ver­sion is some­thing in­ter­nal to the Blob class. If it's a pub­lic in­stance method, it should be unit test­ed; as sim­ple as that. If, on the oth­er hand, it is a pri­vate sta­t­ic one, it can­not be test­ed di­rect­ly, but will be through the unit tests of the pub­lic mem­bers of the class which rely on the con­ver­sion.

The first vari­ant (pub­lic in­stance mem­ber) is straight­for­ward in terms of tests failed af­ter a re­gres­sion: one or sev­er­al tests which point to the method it­self would fail, thus di­rect­ly show­ing the lo­ca­tion of the prob­lem. The sec­ond vari­ant (pri­vate sta­t­ic mem­ber) is slight­ly more dif­fi­cult, as the failed tests would show only the meth­ods which rely on the code which has a re­gres­sion, and it's up to the de­vel­op­er to find which one is that. There is also a third vari­ant: pub­lic in­stance mem­ber which is also used by oth­er pub­lic in­stance mem­bers. Here, you'll have a bunch of tests which failed: some would pin­point to the faulty mem­ber, but oth­ers would point to the mem­bers which only rely on the faulty one.

Both sec­ond and third cas­es are nasty, be­cause they pre­sent sev­er­al failed tests and let you chose among them, and also be­cause you would like­ly spend time de­bug­ging. Un­for­tu­nate­ly, there is no com­mon ap­proach to solve this is­sue. One may hope that the class is rel­a­tive­ly small (and if it's not, there is pos­si­bly an is­sue with the ar­chi­tec­ture), or the diff for the file which cor­re­sponds to the class is small. In most cas­es, the pre­cise lo­ca­tion of the prob­lem will be found quick­ly enough by glanc­ing at the diff. In oth­er words, any failed test will guide the de­vel­op­er to a spe­cif­ic file, and the diff of this file will like­ly guide to one or sev­er­al lines. Not bad.

Ex­am­ple from the pro­ject 2

Let's now imag­ine an­oth­er pro­ject, which is a busi­ness ap­pli­ca­tion which man­ages some en­ti­ties with­in a com­pa­ny. It re­lies on the con­ver­sion from bi­na­ry to hexa­dec­i­mal in four lo­ca­tions:

  1. To log small pieces of bi­na­ry data. The log­ging for­mat is han­dled by a spe­cif­ic log­ging mid­dle­ware which is able to de­cide which data ex­act­ly should be con­vert­ed to hexa­dec­i­mal.

  2. To trace in­put and out­put of all the calls to a quite un­re­li­able third-par­ty ser­vice, in or­der to re­port the is­sues to the ser­vice provider and be able to prove to the provider that the ser­vice is mis­be­hav­ing. Since the ser­vice oc­ca­sion­al­ly sends or re­ceives bi­na­ry data, this data should be con­vert­ed to hu­man­ly-read­able (in what world ex­act­ly hexa­dec­i­mal rep­re­sen­ta­tion could be con­sid­ered hu­man­ly-read­able ?...) form be­fore be­ing stored in trace files.

  3. The ap­pli­ca­tion has a no­tion of user roles, and one of the roles is a sys­tem ad­min­is­tra­tor, who is able to see with­in the UI it­self some iden­ti­fiers dis­played in hexa­dec­i­mal form.

  4. And fi­nal­ly, the ap­pli­ca­tion some­times need to com­pare pieces of data (in­clud­ing bi­na­ry), and the third-par­ty diff li­brary which is used by the ap­pli­ca­tion crash­es mis­er­ably when en­coun­ter­ing the char­ac­ter 0x00. There­fore, the bi­na­ry data is first con­vert­ed to hexa­dec­i­mal, and only then passed to the diff li­brary.

Here, con­ver­sion is used at sev­er­al lo­ca­tions, mean­ing that that ap­proach which was used in the pro­ject 1 doesn't work there. Let's start by a naive ap­proach of a pub­lic sta­t­ic method in a sta­t­ic class. This method is used across the ap­pli­ca­tion, and every­thing seems to be fine, un­less two things hap­pen.

Re­gres­sions

At some point, some­one mod­i­fies the im­ple­men­ta­tion of the con­ver­sion. The change in­tro­duces a re­gres­sion. No big deal, the code is test­ed, so once the de­vel­op­er com­mits his changes, the build fails, re­port­ing six­ty-five failed tests. Among those six­ty-five, there are, some­where, the three ones which cov­er the pub­lic sta­t­ic method, which means that you have 5% chance open­ing one of those, and, in­verse­ly, 95% risk of open­ing one which will point you to a class which re­lies on the con­ver­sion. And if the code is in a bad shape, the class which re­lies on the con­ver­sion may not di­rect­ly call the con­ver­sion; in­stead, it will call some method from an­oth­er class, which would then call... well, you see the pic­ture. Per­son­al­ly, I don't want to be that de­vel­op­er who finds six­ty-five failed tests at 7 p.m. on Fri­day.

Change re­quests

Pos­si­bly even worse is a re­quest to have a dif­fer­ent im­ple­men­ta­tion for one of the lo­ca­tions where the con­ver­sion is used. One could ex­pect a re­sult such as 09c3b2, or 09C3B2, or 0x09C3B2, or the one I ab­solute­ly hate, but which is the only one avail­able in .NET Frame­work: 09-C3-B2.

Imag­ine the prod­uct own­er tells that in the UI, the val­ue should be dis­played in all cap­i­tals. At the same time, there is a dis­cus­sion that in the logs, the val­ue should be pre­fixed with “0x” to make it clear that what fol­lows is a hexa­dec­i­mal val­ue.

One way—and pro­gram­mers who dare do that should be fired—is to add a ToUpper in the UI lay­er, and prepend "0x" string in the logs. This is wrong, be­cause one is in­creas­ing the risk of hav­ing sub­tle bugs lat­er on. For in­stance, imag­ine that six months lat­er, an­oth­er pro­gram­mer is asked to add "0x" pre­fix every­where. No­body tells him that there is al­ready such pre­fix in the logs, and he doesn't spot it when look­ing at the code it­self, so he adds the pre­fix in the com­mon pub­lic sta­t­ic class. And then, a few weeks lat­er, some­one is won­der­ing what's the heck with all those 0x0x09c3b2 in the logs.

An­oth­er way is to split the code, i.e. to refac­tor it in or­der to have dif­fer­ent con­ver­sions for dif­fer­ent us­ages. The pub­lic sta­t­ic Conversion­Utility.Binary­To­Hex will be re­placed by three pub­lic sta­t­ic meth­ods:

If the ini­tial method was used di­rect­ly (i.e. the ac­tu­al code which needs to con­ver­sion calls the method), such refac­tor­ing will be rel­a­tive­ly easy. If, on the oth­er hand, the code is tan­gled, such refac­tor­ing could quick­ly turn into night­mare. Es­pe­cial­ly il­lus­tra­tive is the case where two pieces of code which now need dif­fer­ent re­sults rely on com­mon code which, in turn, calls the con­ver­sion method. For in­stance, be­ing log­i­cal­ly close, the log­ging code and the code which traces the in­puts and out­puts from and to the un­re­li­able third-par­ty ser­vice may be im­ple­ment­ed as de­rived class­es of a com­mon base class. This base class and only it was call­ing the util­i­ty. In this case, the refac­tor­ing would need to touch to the base class and the de­rived class­es.

Com­pare it now with the DI ap­proach. Orig­i­nal­ly, you have a IBin­To­Hex­Converter with a sin­gle mem­ber, Convert, which takes a se­quence of bytes, and re­turns a string. The in­ter­face is im­ple­ment­ed by, on one hand, a Bin­To­Hex­Converter, and on the oth­er hand, by a Bin­To­Hex­Converter­Stub. The first one, nat­u­ral­ly, con­tains the ac­tu­al im­ple­men­ta­tion. The stub con­tains some ba­sic map­ping be­tween hard-cod­ed val­ues (it may be scoped to the con­sumers as well, so for in­stance the thing which sends the val­ues to the UI has its own stub, while the log­ging code has its own).

Re­gard­ing the re­gres­sions, if I break the con­ver­sion with­in the Bin­To­Hex­Converter, I get my three failed tests on the next build. Not six­ty-five; three.

Re­gard­ing the change re­quests, I can just pop up ad­di­tion­al im­ple­men­ta­tions. First, re­name Bin­To­Hex­Converter to Bin­To­Lower­Hex­Converter. Then add Bin­To­Lower­Prefixed­Hex­Converter and Bin­To­Upper­Hex­Converter. When it comes to the lo­ca­tion where two de­rived class­es are re­ly­ing on the base class to call the con­ver­sion, the change is ab­solute­ly straight­for­ward: pass a dif­fer­ent im­ple­men­ta­tion to the de­rived class.

In essence, the DI ap­proach is a tiny bit more com­plex at the be­gin­ning: in­ject­ing a de­pen­den­cy re­quires more code than sim­ply call­ing a sta­t­ic mem­ber of an­oth­er class. This be­ing said, even at the be­gin­ning, DI al­ready brings a ben­e­fit of mak­ing the de­pen­den­cies very clear. By look­ing at the list of in­ter­faces it re­quires, I im­me­di­ate­ly know that it con­verts bi­na­ry data to hexa­dec­i­mal.

The full ben­e­fit, how­ev­er, ap­pears lat­er, i.e. as soon as we start adding dif­fer­ent im­ple­men­ta­tions. The fact that we are rea­son­ing in terms of in­ter­faces here is also ben­e­fi­cial. Imag­ine that I need to add con­ver­sion to Base64. No big deal. The same in­ter­face re­mains; I just re­name IBin­To­Hex­Converter to IBinary­Converter, and add an im­ple­men­ta­tion for Base64.

It is com­mon to be­lieve that this ap­proach makes refac­tor­ing more dif­fi­cult com­pared to the ap­proach where code was call­ing sta­t­ic meth­ods di­rect­ly. In re­al­i­ty, it gets eas­i­er. This ap­plies both to the code, as well as the tests them­selves: you es­sen­tial­ly mod­i­fy only the tests which cov­er the code you're chang­ing. You may have no­ticed that in my ex­am­ple of a change of for­mat from 09c3b2 to 0x09c3b2, tests weren't af­fect­ed at all. An im­ple­men­ta­tion was swapped. So what? It's not of the busi­ness of unit tests. In­te­gra­tion tests? Sure, they will be im­pact­ed. But this is a dif­fer­ent sto­ry. At the unit test lev­el, the stub used as a re­place­ment for the bi­na­ry con­vert­er would still re­turn the same val­ues as be­fore; since it's a stub, the val­ues it re­turns are dumb, and un­re­lat­ed to the ac­tu­al for­mat which is used dur­ing run­time. For in­stance, the stub may just re­turn the string "012345" when be­ing called, in­de­pen­dent­ly of the ac­tu­al in­put val­ue.

On the op­po­site, if one was us­ing a sta­t­ic method, a change in the for­mat would prop­a­gate to the six­ty-five tests (maybe less, de­pend­ing on the change).

Of course, there are sit­u­a­tions where a nasty in­ter­face change has se­vere cas­cad­ing im­pact, re­quir­ing to change lit­er­al­ly hun­dreds of class­es and all the cor­re­spond­ing tests (and the stubs). An ex­am­ple of such change would be the need to in­tro­duce a pos­si­bil­i­ty to de­crypt data. In oth­er words, in pro­ject 2, all data is now en­crypt­ed, and it is re­quired to first de­crypt it, and only then con­vert it to hexa­dec­i­mal for­mat. This would re­quire IBinary­Con­verter.Con­vert(Blob) to be re­placed by IBinary­Con­verter.Con­vert(Blob, IDe­crypter). How­ev­er, if we were us­ing pub­lic sta­t­ic mem­bers, the change would still be rad­i­cal.