Hi Jacques, I am interested first and foremost in cleaning the code base which is hard work and i don't want to be sidetracked by other tasks.
The links below confirm my concerns that the code is problematic, and after your feedback as well as Jacopo's I am more inclined to remove the code than work _around_ it. I am just not sure what to do next or whether to request a vote or just go ahead and try to isolate the code away from the startup component. FYI I have a ready patch that can remove the whole thing, so my hesitation is only in terms ok getting the thumbs up. Suggestions? Taher Alkhateeb On May 27, 2016 8:33 AM, "Jacques Le Roux" <[email protected]> wrote: > Hi Taher, > > While at it, FYI: I'd love to see this flight > https://issues.apache.org/jira/browse/INFRA-3590 > > See also https://issues.apache.org/jira/browse/OFBIZ-4757 > > Jacques > > > Le 26/05/2016 à 19:24, Taher Alkhateeb a écrit : > >> Hi Adam >> >> Ok your objection is a good enough "no" for me to back off. I will try to >> think of a way to work on the integration as you suggested. >> >> I am not picking specifically on your work. However the entire startup >> logic has many problems and lengthy messy code scattered all over the >> place. I am trying as much as I can to "cut out" code until some one says >> no like your good self. >> >> I will fix the build.xml to ensure correct behavior in cobertura. >> >> Taher Alkhateeb >> >> On Thursday, 26 May 2016, Adam Heath <[email protected]> wrote: >> >> Let me restate, do not remove the code coverage tool; fix the integration. >>> >>> Every single time I used code coverage to design more tests, I *always* >>> found bugs. Real bugs. And, I also found unreachable code. I'll give >>> an >>> example: >>> >>> == >>> public void printMap(Map value) { >>> if (value == null) { >>> return; >>> } >>> String foo = safeToString(value); >>> System.err.println(foo); >>> } >>> >>> private String safeToString(Object value) { >>> if (value == null) { >>> return null; >>> } >>> return value.toString(); >>> } >>> == >>> >>> Granted, the above unreachable code in safeToString *code* be discovered >>> with deep study, but an *automated* tool makes it much easier to find. >>> And, the above example is a *very* simple example. Please take a look at >>> the test cases for UtilCache. >>> >>> On 05/26/2016 11:26 AM, Adam Heath wrote: >>> >>> Not to Pierre, but ugly and broken? How so? Please expand with concrete >>>> issues. >>>> >>>> ps: I'm the original integrator of cobertura into ofbiz. >>>> >>>> pps: I have a local branch that converted ofbiz to maven, and actually >>>> produced a runnable output. Should I revive that? >>>> >>>> On 05/26/2016 07:54 AM, Pierre Smits wrote: >>>> >>>> +1 as it never got off the ground properly. We can always revisit later >>>>> when desire to do so rises again. >>>>> >>>>> I use Sonar, but that is another subject. >>>>> >>>>> Best regards, >>>>> >>>>> Pierre Smits >>>>> >>>>> ORRTIZ.COM <http://www.orrtiz.com> >>>>> OFBiz based solutions & services >>>>> >>>>> OFBiz Extensions Marketplace >>>>> http://oem.ofbizci.net/oci-2/ >>>>> >>>>> On Thu, May 26, 2016 at 2:46 PM, Taher Alkhateeb < >>>>> [email protected] >>>>> >>>>> wrote: >>>>>> Hello everyone, >>>>>> >>>>>> As part of the refactoring process, I suggest to completely remove >>>>>> cobertura and sonar from the framework. My proposal is based on the >>>>>> following: >>>>>> >>>>>> - The startup logic is more complex because of the existence of legacy >>>>>> classes (Instrumenter, InstrumenterWorker, etc ...). >>>>>> - No one (AFAIK) is actively using cobertura or sonar, and the targets >>>>>> in >>>>>> build.xml are actually broken >>>>>> - The way cobertura is integrated with ofbiz is poor and ugly >>>>>> - Before integrating cobertura, ofbiz first needs a better testing >>>>>> framework that allows for TDD and red-green-refactor. Otherwise, this >>>>>> whole >>>>>> issue with test coverage is a moot point >>>>>> - Too much complexity and legacy code in build.xml, common.xml, >>>>>> ivy.xml, >>>>>> macros.xml and others. It's just really ugly >>>>>> >>>>>> All the code that I saw for cobertura is just ugly and broken. Now >>>>>> it's >>>>>> perfectly fine to reintroduce cobertura cleanly in the future, but I >>>>>> would >>>>>> not use the existing code anyway, I would just wipe it all out and >>>>>> start >>>>>> fresh. >>>>>> >>>>>> I'm not sure whether we need to vote on this? Appreciate your >>>>>> feedback. >>>>>> >>>>>> Taher Alkhateeb >>>>>> >>>>>> >>>>>> >
