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
