+1
On 2020/03/02 16:52:08 Daniel Watford wrote: > Hi Mathieu, > > Yes I agree that mocking of static methods is an indicator of refactoring > needed. > > There are always exceptions to every rule, but in this case with > MacroFormRenderer longer than > 3000 lines in R18 and its role is to map from ModelFormField instances to > string representation > of FTL macro calls, and then to execute those FTL macros, it feels like an > excellent candidate for a > bit of refactoring. > > However, to do so without any sort of test framework in place will be > introducing risk. > > At first a refactoring effort might seem simple, but with such a large > class we are likely to find > some difficulties. Therefore if I had a vote I would suggest accepting the > JMockit based tests with > a view to retire and replace them with more standard and recognisable > Mockito tests once > made possible through refactoring. > > If the committers accept https://github.com/apache/ofbiz-framework/pull/26 > and > https://github.com/apache/ofbiz-framework/pull/27 I will be happy to open > and work on a ticket to > refactor MacroFormRenderer and associated tests. I'd aim to deliver any > refactoring as a number > of small patches/PRs over a period of time. > > Thanks, > > Dan. > > On Mon, 2 Mar 2020 at 16:17, Mathieu Lirzin <mathieu.lir...@nereide.fr> > wrote: > > > Hello Daniel, > > > > Daniel Watford <d...@foomoo.co.uk> writes: > > > > > I don't have a preference between the two mocking libraries, but creating > > > unit tests for MacroFormRenderer necessitates mocking static methods. > > > Mockito doesn't offer static method mocking, but JMockit does. > > > [...] > > > To provide unit tests for MacroFormRenderer without using a mocking > > library > > > like JMockit will require the class to be refactored which I deemed a bit > > > of a risky change at this point, particularly without existing tests to > > > prove correct behaviour is maintained. > > > > Any effort that take into account how things can effectively be tested > > is a good thing for OFBiz. :-) > > > > My impression is that the "need" to mock static methods is a symptom of > > entangled code. And that making the code simpler and better designed > > will remove that need. As you said having to refactor without tests in > > the first place is not good but usage of advanced mocking techniques to > > workaround the fact that basic unit testing is too hard is not ideal > > neither. > > > > Another solution (that is not ideal neither) is to rely on manual > > testing in the first place (or automatic integration tests if you have > > time) introduce the architectural refactoring and then add you unit test > > with explicit arguments based dependency injection instead of mocks. > > > > In any case that is a matter of compromise. :-) > > > > -- > > Mathieu Lirzin > > GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37 > > > > > -- > Daniel Watford >
