Re: StreamsEqual issue(s)

> -----Original Message-----
> From: [email protected]
[mailto:[email protected]] On Behalf Of Charlie
Poole
> Sent: woensdag 13 augustus 2008 21:21
> To: 'Gert Driesen'; [email protected]
> Subject: Re: [nunit-developer] StreamsEqual issue(s)

Hey Charlie,

>
> > > > I also noticed NUnit is changing the position in the 
> > stream (to the 
> > > > start of the stream), and doesn't change it back 
> > afterwards. Should 
> > > > an Assert by itself be allowed to change the state of a stream?
> > >
> > > In some cases, NUnit actually creates the stream by opening a file. 
> > > But in others, it may be passed in as an argument.
> > > Theoretically, we should not change it or at least document that it 
> > > may be changed. I'll look at it.
> > 
> > It the stream is both readable and seekable, then I think 
> > NUnit should store the initial position, and reposition back 
> > at that position after comparing the content of the stream.
>
> That makes sense - I'll put it into the head anyway.

Great. 

> > > > I'm also not sure if NUnit should actually read these 
> > streams. What 
> > > > will you do if the stream is readable and not seekable? 
> > You'd change 
> > > > the position in the stream (by positioning at the 
> > beginning and by 
> > > > reading it), and you can't change it back to the origin position.
> > >
> > > I can't think of any good use case for wanting to assert on the 
> > > equality of two non-seekable streams. Can you? Maybe I should just 
> > > throw an InvalidArgumentException.
> > 
> > No, but if you aim for backward compatibility then this is 
> > not the best approach.
>
> My logic so far has been that it's OK to change behavior if it
> only impacts something that should not have been acceptable in
> the first place. Since NUnit is a /test/ framework, and isn't
> (normally) shipping as part of the software being tested, pointing
> out a problem is a good thing.

Yes, I agree.

> > But well, the change from Stream.Equals to actually comparing 
> > the content of the streams isn't either.
> > 
> > I guess this does not apply to only Streams. I leave it up to 
> > your good judgement to decide which approach to take.
>
> In fact, it applies anyplace where we have added new behavior
> to AreEqual(object, object). There's a tradeoff but I think it's
> largely in favor of making such changes when they are useful
> because,
>
> 1) They don't generally break anything but a test for reference
> equality.
>
> 2) There's a preferred way to do that.

I understand. The Stream case is probably somewhat unique, and the test
itself was kinda broken too.

I think it's sufficient to document this (and add a unit test for the
behavior with non-readable and/or non-seekable streams).

> > > > Apart from this, I wondered why NUnit is not always checking for 
> > > > reference equality first? Why bothering checking the 
> > content of two 
> > > > streams, it they are actually the same?
> > >
> > > But why is the user bothering to use the Assert in that case? 
> > > He should be using AreSame rather than AreEqual. OTOH, I 
> > can imagine 
> > > odd cases where it would not be possible to know in 
> > advance, so it's 
> > > worth a check.
> > 
> > He should indeed be using AreSame, but this specific test 
> > even dated from before Assert.AreSame existed (the fixture 
> > derived from Assertion, and used AssertEquals to compare the 
> > streams). There are actually quite a few tests in Mono that 
> > use this (long-time) deprecated construct.
>
> So maybe the first patch is to upgrade the tests to NUnit 2.2. :-)

If I ever get bored for a few weeks, I'll take care of that :p

> > > I wasn't planning to do any more development in the 2.4 
> > code line, but 
> > > I guess I could do a fix for mono if there turn out to be 
> > any issues 
> > > that require it. Alternatively, you may want to look at 2.5, which 
> > > will get active maintenance after it is released.
> > 
> > I don't think you should bother fixing this for 2.4. I'll 
> > even need to check with Miguel if he wants to consider 
> > upgrading NUnit.
>
> I suggest running your same tests with the 2.5 alpha to see if
> there are any further problems. I suspect there will not be, but
> I can easily fix any incompatibilities there.

I could consider this, but I doubt a patch to upgrade to an alpha version of
NUnit will be accepted.

Perhaps the other issues I found are also just broken tests. If I recall
correctly one was about comparing floats.

> > In fact, fixing anything that made it easier to use NUnit in
> > Mono would be a big priority.
>
> BTW, do you think I should have some sort of install for linux?
> Up to now it has been totally left to the packagers.

Yeah, probably. For NAnt, the Novell team creates RPMs. But I think each
open-source project should take care of this themselves to avoid becoming
dependent on the goodwill and schedule of Novell.

For NAnt, the problem was that I did not know enough about linux packaging,
and did not have enough time (or interest) to look into it.

Gert


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/