Login  Register

Re: Re: OFBIZ-4035: change the id Attribute for input fields in the model macro form renderer from String to FlexibleStringExpander

Posted by James Yong-2 on Mar 02, 2020; 4:59pm
URL: http://ofbiz.116.s1.nabble.com/OFBIZ-4035-change-the-id-Attribute-for-input-fields-in-the-model-macro-form-renderer-from-String-to-r-tp4746130p4747101.html

+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 <[hidden email]>
> wrote:
>
> > Hello Daniel,
> >
> > Daniel Watford <[hidden email]> 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
>