Re: StreamsEqual issue(s)

> -----Original Message-----
> From: [email protected]
[mailto:[email protected]] On Behalf Of Charlie
Poole
> Sent: woensdag 13 augustus 2008 18:59
> To: 'Gert Driesen'; [email protected]
> Subject: Re: [nunit-developer] StreamsEqual issue(s)
>
> Hi Gert,
>
> Hey! I'm glad someone is looking at that. :-) Are you looking
> to test Mono itself with 2.4.8 or just ship with it?

My intention is to test Mono with it, but since it's also used for
Grasshopper (JVM) someone on that team will also need to look into the
upgrade.

So I'm not sure if my patch for upgrading to 2.4.8 will even be accepted. As
I've said, there are also other issues that surfaced after the upgrade that
would need to be fixed first. 

> I'm not real familiar with that constraint, but your points
> make sense.  I'll look it over.
>
> More inline...
>
> > I was having a quick look at the impact of upgrading the 
> > version of NUnit in Mono from 2.2 to 2.4.8, and noticed some 
> > test regressions.
> > 
> > I had a closer look at one of these regressions, and the 
> > reason it failed is because EqualConstraint.StreamsEqual 
> > expects both streams to be readable and seekable. If not, 
> > this results in an exception.
>
> So your test was previously comparing two streams with
> Assert.AreEqual? Since NUnit had no prior code for streams,
> you would be getting Stream.Equals (i.e. Object.Equals).
> The easiest fix would be to use Assert.SameAs if that's
> what you want.

It wasn't one of my tests, but the workaround would indeed be to use
Assert.AreSame (or similar).

> > I don't think an assert should result in an exception (other 
> > than AssertionException.cs).
>
> Agreed... plus any ArgumentInvalid, etc. that are the fault
> of the user.

Yes, of course.

> > 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.

> > 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.

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.

> > 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.

> 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.

Perhaps he prefers to keep things as is.

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=/