Re: svn commit: r905696 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java

[Adam Heath <do...@brainfood.com>]

adrianc@apache.org wrote:
> Author: adrianc
> Date: Tue Feb  2 16:35:28 2010
> New Revision: 905696
> 
> URL: http://svn.apache.org/viewvc?rev=905696&view=rev
> Log:
> Made a small change to FlexibleStringExpander - created a NullElem class to support existing client code that tested getOriginal() for null. By the way, that is a bad practice - use the isEmpty() method instead.
> 
> Problem reported by Scott and Hans on the dev mailing list.

Test case?

You should write a bunch of test cases against FSE, being certain to
not use any internal methods or classes.  Then, revert back to older
FSE, and run the same test suite, to see if it still functions.

I did that once with our COW filesystem.  It was running too slow, and
I couldn't find any obvious flaws.  I then rewrote it(without test
cases), giving it a new name and new classes.  The site access went
from 13req/s to 500+req/s when I did this.  However, I wasn't
comfortable switching all deployed things to it yet.

So, I wrote a single test suite, that could test either version, and
used cobertura to ensure that I was actually accessing most lines and
branches in both classes.  Then I felt comfortable switching
everything in mass.

I think that a proper FSE-only test suite would have discovered this
in the first place.  If the old version allowed getOriginal() to
return null, then the new version would have failed the test.

Re: svn commit: r905696 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java

[David E Jones <de...@me.com>]

On Feb 2, 2010, at 10:47 AM, Adam Heath wrote:

> adrianc@apache.org wrote:
>> Author: adrianc
>> Date: Tue Feb  2 16:35:28 2010
>> New Revision: 905696
>> 
>> URL: http://svn.apache.org/viewvc?rev=905696&view=rev
>> Log:
>> Made a small change to FlexibleStringExpander - created a NullElem class to support existing client code that tested getOriginal() for null. By the way, that is a bad practice - use the isEmpty() method instead.
>> 
>> Problem reported by Scott and Hans on the dev mailing list.
> 
> Test case?
> 
> You should write a bunch of test cases against FSE, being certain to
> not use any internal methods or classes.  Then, revert back to older
> FSE, and run the same test suite, to see if it still functions.
> 
> I did that once with our COW filesystem.  It was running too slow, and
> I couldn't find any obvious flaws.  I then rewrote it(without test
> cases), giving it a new name and new classes.  The site access went
> from 13req/s to 500+req/s when I did this.  However, I wasn't
> comfortable switching all deployed things to it yet.
> 
> So, I wrote a single test suite, that could test either version, and
> used cobertura to ensure that I was actually accessing most lines and
> branches in both classes.  Then I felt comfortable switching
> everything in mass.
> 
> I think that a proper FSE-only test suite would have discovered this
> in the first place.  If the old version allowed getOriginal() to
> return null, then the new version would have failed the test.

It might have, but only if the author of the test suite anticipated this issue. If the author was not aware of this issue or able to predict the future in this way, then it wouldn't do ANY good whatsoever, except perhaps make people more confident that there are no issues, which may lead people away from finding the actual issue... ;)

-David