Daniel Watford <d...@foomoo.co.uk> writes: > 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.
I feel the pain. > However, to do so without any sort of test framework in place will be > introducing risk. You are right. If you are fearful of the code you touch (which is legitimate in this case) indeed adding some test automation is a nice idea. > 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 AFAIC I think the unbearable complexity and awfulness of the implementation FTL/Screen rendering justifies adopting complex testing techniques and adding a new even partially redundant dependency like JMockit, even if this is sad to have to do so. PS: Votes in regards code modification in a community with PMC members that basically disagree on everything else than “We are an awesome community” is not a solution to anything because any veto with a valid technical decision like for example “JMockit is redundant with Mockito purpose which is already used” would simply block your proposition, so a vote for code modification serves only to validate a consensus. [1] [1] https://www.apache.org/foundation/voting.html#votes-on-code-modification > with a view to retire and replace them with more standard and > recognisable Mockito tests once made possible through refactoring. I would suggest seeking the goal of adopting basic dependency injection with explicit method/constructor arguments which leads to simpler tests and is a sign of well designed code, instead of seeking the replacement of advanced mocking techniques with less advanced ones. Mocking is sometimes useful but should be avoided when possible. > 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. Good luck. -- Mathieu Lirzin GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
