You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Tim Ellison <t....@gmail.com> on 2009/11/13 18:22:28 UTC

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

This commit breaks a couple of existing compatibility tests for
BufferedReader:

----------------------------------------
java.lang.NullPointerException: buffer is null
at java.io.BufferedReader.read(BufferedReader.java:282)
at
org.apache.harmony.luni.tests.java.io.BufferedReaderTest.test_read_$CII_Exception(BufferedReaderTest.java:382)
at java.lang.reflect.VMReflection.invokeMethod(VMReflection.java)
----------------------------------------

and

----------------------------------------
junit.framework.AssertionFailedError: should throw IOException
at
org.apache.harmony.luni.tests.java.io.BufferedReaderTest.test_reset_IOException(BufferedReaderTest.java:538)
at java.lang.reflect.VMReflection.invokeMethod(VMReflection.java)
----------------------------------------

Regards,
Tim

On 12/Nov/2009 03:25, jessewilson@apache.org wrote:
> Author: jessewilson
> Date: Thu Nov 12 03:25:52 2009
> New Revision: 835212
> 
> URL: http://svn.apache.org/viewvc?rev=835212&view=rev
> Log:
> Cleanup and bugfix BufferedReader.
> 
> This change includes the following functional changes:
>  - changing read to not clear the mark upon reading EOF. The previous behaviour was incorrect.
>  - changing read(char[], int, int) to use the 'read directly from the source stream' shortcut when the mark has exceeded its limit. Previously we took the shortcut only when the mark was unset.
> 
> And these nonfunctional changes:
>  - rewrote read(char[], int, int). The new revision contains only one call to 'System.arrayCopy' and the related bookkeeping. Previously there was one call before the loop, and another call in the loop.
>  - renamed markpos to mark
>  - renamed marklimit to markLimit
>  - renamed count to end (it isn't a count, it's a position)
>  - renamed fillbuf() to fillBuf()
>  - simplified conditions that used >= when > was impossible
>  - reducing the number of field reads where convenient
> 
> This tidy up is intended to prepare BufferedReader for some further bugfixing. We've seen some bugs reported against readLine() and I found it quite frustrating to work on the code when the names were wrong (ie. count) or of a foreign style (such as fillbuf()). I also attempted to document what the heck was going on in the more sophiticated methods.
> 
> Modified:
>     harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Posted by Regis <xu...@gmail.com>.
Alexei Fedotov wrote:
> Funny. We don't have a requirement that a committer have to test the
> code before committing it here [1].
> 
> [1] http://wiki.apache.org/harmony/NewCommitter
> 
> 

This page just point out the steps to configure develop tools for a new 
committer. Testing code is what *every* contributor must do before contributing 
code to community.

-- 
Best Regards,
Regis.

Re: [testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)

Posted by Mark Hindess <ma...@googlemail.com>.
In message <4B...@gmail.com>, Tim Ellison writes:
>
> On 17/Nov/2009 03:50, Nathan Beyer wrote:
> > 2009/11/16 Alexei Fedotov <al...@gmail.com>:
> >> Funny. We don't have a requirement that a committer have to test the
> >> code before committing it here [1].
> > 
> > I suppose some things are just assumed.
> > 
> >> [1] http://wiki.apache.org/harmony/NewCommitter
> 
> Of course, we are all grown-ups.  Mistakes happen.
> 
> How close are we to being able to run all the tests via Hudson?
> I recall that there was discussion and work on packaging the tests so
> they can be run from the HDK.  I don't recall it being declared 'done'?

"ant -f /path/to/hdk/build/test/build.xml" should work but there are
a few tests that fail when run this way (which is a bit odd since the
tests effectively run from there when you run "ant test" too.  However,
I see consistently broken tests anyway so perhaps we should just start
running all the tests this way *and* investigate all of the failures
then start being stricter about keeping them working (or excluded).

Regards,
 Mark.

Re: [testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)

Posted by Alexei Fedotov <al...@gmail.com>.
Ok, done.


On Wed, Nov 18, 2009 at 6:11 AM, Nathan Beyer <nb...@gmail.com> wrote:
> 2009/11/17 Alexei Fedotov <al...@gmail.com>:
>> I would rather call this "misunderstanding".
>>
>> Jesse run one set of tests, Tim expected another set. One way to
>
> If you think it should be documented on the wiki, go ahead and do it -
> it's open and intended to be less formal than the proper web site.
>
> -Nathan
>
>> correct the problem is to agree that committers run a specific minimal
>> set of tests, for example, on one platform. I thought we had such
>> agreements documented, but failed to find a proper pointer. It seems
>> to me now that instead of formal pre-commit rules we had Stepan
>> Mishura who guided us informally.
>>
>>
>>
>>
>> On Tue, Nov 17, 2009 at 6:38 PM, Tim Ellison <t....@gmail.com> wrote:
>>> On 17/Nov/2009 03:50, Nathan Beyer wrote:
>>>> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>>>>> Funny. We don't have a requirement that a committer have to test the
>>>>> code before committing it here [1].
>>>>
>>>> I suppose some things are just assumed.
>>>>
>>>>> [1] http://wiki.apache.org/harmony/NewCommitter
>>>
>>> Of course, we are all grown-ups.  Mistakes happen.
>>>
>>> How close are we to being able to run all the tests via Hudson?
>>> I recall that there was discussion and work on packaging the tests so
>>> they can be run from the HDK.  I don't recall it being declared 'done'?
>>>
>>> Regards,
>>> Tim
>>>
>>
>>
>>
>> --
>> With best regards / с наилучшими пожеланиями,
>> Alexei Fedotov / Алексей Федотов,
>> http://www.telecom-express.ru/
>> http://harmony.apache.org/
>> http://www.expressaas.com/
>> http://openmeetings.googlecode.com/
>>
>



-- 
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/

Re: [testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)

Posted by Nathan Beyer <nb...@gmail.com>.
2009/11/17 Alexei Fedotov <al...@gmail.com>:
> I would rather call this "misunderstanding".
>
> Jesse run one set of tests, Tim expected another set. One way to

If you think it should be documented on the wiki, go ahead and do it -
it's open and intended to be less formal than the proper web site.

-Nathan

> correct the problem is to agree that committers run a specific minimal
> set of tests, for example, on one platform. I thought we had such
> agreements documented, but failed to find a proper pointer. It seems
> to me now that instead of formal pre-commit rules we had Stepan
> Mishura who guided us informally.
>
>
>
>
> On Tue, Nov 17, 2009 at 6:38 PM, Tim Ellison <t....@gmail.com> wrote:
>> On 17/Nov/2009 03:50, Nathan Beyer wrote:
>>> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>>>> Funny. We don't have a requirement that a committer have to test the
>>>> code before committing it here [1].
>>>
>>> I suppose some things are just assumed.
>>>
>>>> [1] http://wiki.apache.org/harmony/NewCommitter
>>
>> Of course, we are all grown-ups.  Mistakes happen.
>>
>> How close are we to being able to run all the tests via Hudson?
>> I recall that there was discussion and work on packaging the tests so
>> they can be run from the HDK.  I don't recall it being declared 'done'?
>>
>> Regards,
>> Tim
>>
>
>
>
> --
> With best regards / с наилучшими пожеланиями,
> Alexei Fedotov / Алексей Федотов,
> http://www.telecom-express.ru/
> http://harmony.apache.org/
> http://www.expressaas.com/
> http://openmeetings.googlecode.com/
>

Re: [testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)

Posted by Alexei Fedotov <al...@gmail.com>.
I would rather call this "misunderstanding".

Jesse run one set of tests, Tim expected another set. One way to
correct the problem is to agree that committers run a specific minimal
set of tests, for example, on one platform. I thought we had such
agreements documented, but failed to find a proper pointer. It seems
to me now that instead of formal pre-commit rules we had Stepan
Mishura who guided us informally.




On Tue, Nov 17, 2009 at 6:38 PM, Tim Ellison <t....@gmail.com> wrote:
> On 17/Nov/2009 03:50, Nathan Beyer wrote:
>> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>>> Funny. We don't have a requirement that a committer have to test the
>>> code before committing it here [1].
>>
>> I suppose some things are just assumed.
>>
>>> [1] http://wiki.apache.org/harmony/NewCommitter
>
> Of course, we are all grown-ups.  Mistakes happen.
>
> How close are we to being able to run all the tests via Hudson?
> I recall that there was discussion and work on packaging the tests so
> they can be run from the HDK.  I don't recall it being declared 'done'?
>
> Regards,
> Tim
>



-- 
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/

[testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)

Posted by Tim Ellison <t....@gmail.com>.
On 17/Nov/2009 03:50, Nathan Beyer wrote:
> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>> Funny. We don't have a requirement that a committer have to test the
>> code before committing it here [1].
> 
> I suppose some things are just assumed.
> 
>> [1] http://wiki.apache.org/harmony/NewCommitter

Of course, we are all grown-ups.  Mistakes happen.

How close are we to being able to run all the tests via Hudson?
I recall that there was discussion and work on packaging the tests so
they can be run from the HDK.  I don't recall it being declared 'done'?

Regards,
Tim

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Posted by Nathan Beyer <nb...@gmail.com>.
2009/11/16 Alexei Fedotov <al...@gmail.com>:
> Funny. We don't have a requirement that a committer have to test the
> code before committing it here [1].

I suppose some things are just assumed.

>
> [1] http://wiki.apache.org/harmony/NewCommitter
>
>
> On Fri, Nov 13, 2009 at 10:10 PM, Jesse Wilson <je...@google.com> wrote:
>> On Fri, Nov 13, 2009 at 9:41 AM, Tim Ellison <t....@gmail.com> wrote:
>>
>>> I took a look at the tests briefly, they are checking exception throwing
>>> compatibility, something that we aim to maintain with the RI as rightly
>>> or wrongly apps depend upon non-spec'd behavior.
>>>
>>
>> Sorry about this guys.
>>
>> One of the engineering problems I'm working through is that Android has its
>> own copy of the Harmony test suite. It has additional tests, plus some
>> changes to the original test suite. I'm not particularly happy having two
>> copies! Anyway, in submitting this change I'd run the BufferedReader changes
>> through our test suite, but not Harmony's, and as a consequence I've broken
>> some tests. I'll take better care going forward. The silver lining in this
>> is that Android has many tests to contribute in time.
>>
>> I've committed the fix as r835954.
>> http://svn.apache.org/viewvc?view=revision&revision=835954
>>
>
>
>
> --
> With best regards / с наилучшими пожеланиями,
> Alexei Fedotov / Алексей Федотов,
> http://www.telecom-express.ru/
> http://harmony.apache.org/
> http://www.expressaas.com/
> http://openmeetings.googlecode.com/
>

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Posted by Alexei Fedotov <al...@gmail.com>.
Funny. We don't have a requirement that a committer have to test the
code before committing it here [1].

[1] http://wiki.apache.org/harmony/NewCommitter


On Fri, Nov 13, 2009 at 10:10 PM, Jesse Wilson <je...@google.com> wrote:
> On Fri, Nov 13, 2009 at 9:41 AM, Tim Ellison <t....@gmail.com> wrote:
>
>> I took a look at the tests briefly, they are checking exception throwing
>> compatibility, something that we aim to maintain with the RI as rightly
>> or wrongly apps depend upon non-spec'd behavior.
>>
>
> Sorry about this guys.
>
> One of the engineering problems I'm working through is that Android has its
> own copy of the Harmony test suite. It has additional tests, plus some
> changes to the original test suite. I'm not particularly happy having two
> copies! Anyway, in submitting this change I'd run the BufferedReader changes
> through our test suite, but not Harmony's, and as a consequence I've broken
> some tests. I'll take better care going forward. The silver lining in this
> is that Android has many tests to contribute in time.
>
> I've committed the fix as r835954.
> http://svn.apache.org/viewvc?view=revision&revision=835954
>



-- 
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Posted by Jesse Wilson <je...@google.com>.
On Fri, Nov 13, 2009 at 9:41 AM, Tim Ellison <t....@gmail.com> wrote:

> I took a look at the tests briefly, they are checking exception throwing
> compatibility, something that we aim to maintain with the RI as rightly
> or wrongly apps depend upon non-spec'd behavior.
>

Sorry about this guys.

One of the engineering problems I'm working through is that Android has its
own copy of the Harmony test suite. It has additional tests, plus some
changes to the original test suite. I'm not particularly happy having two
copies! Anyway, in submitting this change I'd run the BufferedReader changes
through our test suite, but not Harmony's, and as a consequence I've broken
some tests. I'll take better care going forward. The silver lining in this
is that Android has many tests to contribute in time.

I've committed the fix as r835954.
http://svn.apache.org/viewvc?view=revision&revision=835954

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Posted by Tim Ellison <t....@gmail.com>.
On 13/Nov/2009 17:27, Jesse Wilson wrote:
> On Fri, Nov 13, 2009 at 9:22 AM, Tim Ellison <t....@gmail.com> wrote:
> 
>> This commit breaks a couple of existing compatibility tests for
>> BufferedReader:
>>
> 
> Yup, I saw this and I'm fixing it. I suspect it's just a bogus test.  Sorry
> about that!

I took a look at the tests briefly, they are checking exception throwing
compatibility, something that we aim to maintain with the RI as rightly
or wrongly apps depend upon non-spec'd behavior.

Regards,
Tim

Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java

Posted by Jesse Wilson <je...@google.com>.
On Fri, Nov 13, 2009 at 9:22 AM, Tim Ellison <t....@gmail.com> wrote:

> This commit breaks a couple of existing compatibility tests for
> BufferedReader:
>

Yup, I saw this and I'm fixing it. I suspect it's just a bogus test.  Sorry
about that!