Re: svn commit: r930737 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java

[Adrian Crum-2]

--- On Mon, 4/5/10, Adam Heath <[hidden email]> wrote:

> Bob Morley wrote:
> >
> > Scott Gray-2 wrote:
> >> On 5/04/2010, at 10:54 PM, Adam Heath wrote:
> >>
> >>> Bob Morley wrote:
> >>>> Thanks for that url; very interesting
> indeed.  From what I could tell
> >>>> the
> >>>> set of unit tests that execute are
> littered with failures (or they
> >>>> intentionally cause a lot stack traces due
> to exception).
> >>> I've been trying to reduce the number of
> duplicated errors logged.
> >>> OfBiz is famous of catching an error, logging
> it, then rethrowing it.
> >>> Repeat ad-infinitum.
> >> OfBiz is famous of catching an error, logging it,
> then rethrowing it.
> >> Repeat ad-infinitum.
> >>
> >
> > Ahh yes; I spotted one simple problem --
> >
> >      [java] Unable to load test suite
> class :
> > org.ofbiz.order.test.CustRequestTest
> >      [java] Exception:
> java.lang.ClassNotFoundException
> >      [java] Message:
> org.ofbiz.order.test.CustRequestTest
> >
> > Looks like if we have a bad testdef configured, we do
> not bubble this up and
> > it gets lost.  This should probably be failing
> the build ...
> >
> > Most of the noise seems to come as you guys indicated,
> from negative test
> > cases coupled with the fact that we are logging
> "info".  Here is a pattern
> > from the bottom of a tester --
> >     
>    assertNotNull("Foreign key referential
> integrity is not observed for
> > create (INSERT)", caught);
> >     
>    Debug.logInfo(caught.toString(), module);
> >
> > The second is explicit rollback and the end of a test
> method which under the
> > covers (if info is turned on) it will create an
> exception and logError with
> > it --
> >     
>    TransactionUtil.rollback(transBegin, null,
> null);
> >
> >
> > I will fix the first problem when I get into the
> office tomorrow (as it is
> > not really pressing).  The second thing though,
> wanna consider changing the
> > log4j threshold on the run-tests target to warn? 
> This would cut down on the
> > noise (but not eliminate).
>
> I just fixed the first.
>
> The others should have their logging fixed, don't change
> the log level
> for run-tests.
>
> If low-level code logs an error, and rethrows, then just
> stop logging
> the error.

Another pattern that bugs me:

try {
    ...
} catch (Exception e) {
    Debug.logError(e, "Something went wrong: " + e.getMessage(), module);
}

The e.getMessage isn't necessary - the message will be printed in the stack trace.

-Adrian

[Scott Gray-2]

On 5/04/2010, at 11:38 PM, Adrian Crum wrote:

> --- On Mon, 4/5/10, Adam Heath <[hidden email]> wrote:
>> Bob Morley wrote:
>>>
>>> Scott Gray-2 wrote:
>>>> On 5/04/2010, at 10:54 PM, Adam Heath wrote:
>>>>
>>>>> Bob Morley wrote:
>>>>>> Thanks for that url; very interesting
>> indeed.  From what I could tell
>>>>>> the
>>>>>> set of unit tests that execute are
>> littered with failures (or they
>>>>>> intentionally cause a lot stack traces due
>> to exception).
>>>>> I've been trying to reduce the number of
>> duplicated errors logged.
>>>>> OfBiz is famous of catching an error, logging
>> it, then rethrowing it.
>>>>> Repeat ad-infinitum.
>>>> OfBiz is famous of catching an error, logging it,
>> then rethrowing it.
>>>> Repeat ad-infinitum.
>>>>
>>>
>>> Ahh yes; I spotted one simple problem --
>>>
>>>       [java] Unable to load test suite
>> class :
>>> org.ofbiz.order.test.CustRequestTest
>>>       [java] Exception:
>> java.lang.ClassNotFoundException
>>>       [java] Message:
>> org.ofbiz.order.test.CustRequestTest
>>>
>>> Looks like if we have a bad testdef configured, we do
>> not bubble this up and
>>> it gets lost.  This should probably be failing
>> the build ...
>>>
>>> Most of the noise seems to come as you guys indicated,
>> from negative test
>>> cases coupled with the fact that we are logging
>> "info".  Here is a pattern
>>> from the bottom of a tester --
>>>      
>>    assertNotNull("Foreign key referential
>> integrity is not observed for
>>> create (INSERT)", caught);
>>>      
>>    Debug.logInfo(caught.toString(), module);
>>>
>>> The second is explicit rollback and the end of a test
>> method which under the
>>> covers (if info is turned on) it will create an
>> exception and logError with
>>> it --
>>>      
>>    TransactionUtil.rollback(transBegin, null,
>> null);
>>>
>>>
>>> I will fix the first problem when I get into the
>> office tomorrow (as it is
>>> not really pressing).  The second thing though,
>> wanna consider changing the
>>> log4j threshold on the run-tests target to warn?
>> This would cut down on the
>>> noise (but not eliminate).
>>
>> I just fixed the first.
>>
>> The others should have their logging fixed, don't change
>> the log level
>> for run-tests.
>>
>> If low-level code logs an error, and rethrows, then just
>> stop logging
>> the error.
>
> Another pattern that bugs me:
>
> try {
>    ...
> } catch (Exception e) {
>    Debug.logError(e, "Something went wrong: " + e.getMessage(), module);
> }
>
> The e.getMessage isn't necessary - the message will be printed in the stack trace.
>
> -Adrian

That's nothing, check out the 80-odd occurrences of this:
try {
    ...
} catch (Exception e) {
    ServiceUtil.returnError(e.getMessage);
}

Not really logging related but wow, 80 occurrences?

[David E. Jones-2]

On Apr 6, 2010, at 12:51 AM, Scott Gray wrote:

> On 5/04/2010, at 11:38 PM, Adrian Crum wrote:
>
>> --- On Mon, 4/5/10, Adam Heath <[hidden email]> wrote:
>>>>
>>>
>>> I just fixed the first.
>>>
>>> The others should have their logging fixed, don't change
>>> the log level
>>> for run-tests.
>>>
>>> If low-level code logs an error, and rethrows, then just
>>> stop logging
>>> the error.
>>
>> Another pattern that bugs me:
>>
>> try {
>>   ...
>> } catch (Exception e) {
>>   Debug.logError(e, "Something went wrong: " + e.getMessage(), module);
>> }
>>
>> The e.getMessage isn't necessary - the message will be printed in the stack trace.
>>
>> -Adrian
>
> That's nothing, check out the 80-odd occurrences of this:
> try {
>    ...
> } catch (Exception e) {
>    ServiceUtil.returnError(e.getMessage);
> }
>
> Not really logging related but wow, 80 occurrences?

This is a good example of error hiding, kind of the opposite of logging an exception multiple times... and much worse as with this pattern there's a good chance you'll never find the source of the error without making a code change to fix the error hiding.

-David

[BJ Freeman]

I use the commits update my 9.04.
I also use the ml to check on what has been done when something goes south.
I will try to look at the commits.
I have about 15 projects I am trying to complete so I am rather busy.

=========================
BJ Freeman
http://bjfreeman.elance.com
Strategic Power Office with Supplier Automation <http://www.businessesnetwork.com/automation/viewforum.php?f=93>
Specialtymarket.com <http://www.specialtymarket.com/>

Systems Integrator-- Glad to Assist

Chat  Y! messenger: bjfr33man
Linkedin
<http://www.linkedin.com/profile?viewProfile=&key=1237480&locale=en_US&trk=tab_pro>


Adam Heath sent the following on 4/5/2010 4:11 PM:

> [hidden email] wrote:
>> Author: doogie
>> Date: Sun Apr  4 19:08:46 2010
>> New Revision: 930737
>>
>> URL: http://svn.apache.org/viewvc?rev=930737&view=rev
>> Log:
>> Make use of UtilObject.equalsHelper in storeAll.
>>
>> Modified:
>>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java?rev=930737&r1=930736&r2=930737&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Sun Apr  4 19:08:46 2010
>> @@ -1450,7 +1450,7 @@ public class GenericDelegator implements
>>                          if (value.containsKey(fieldName)) {
>>                              Object fieldValue = value.get(fieldName);
>>                              Object oldValue = existing.get(fieldName);
>> -                            if ((fieldValue == null && oldValue != null) || (fieldValue != null && !fieldValue.equals(oldValue))) {
>> +                            if (UtilObject.equalsHelper(oldValue, fieldValue)) {
>>                                  toStore.put(fieldName, fieldValue);
>>                                  atLeastOneField = true;
>>                              }
>
> Did anyone else review this commit?  Or did you not understand the code?
>
> Yes, I screwed it up.  But if no one reviews the commits, then how
> will much bigger problems be found?  This was a simple problem to
> find, an obvious inverted test, yet no one noticed it.
>
> What's the point of having commits mailed to the list if no one will
> review them?
>
>