+1 to get ahead :)
Jacques
Le 27/05/2016 à 08:03, Taher Alkhateeb a écrit :
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
