Re: StreamsEqual issue(s)

I looked at the code and realized that the entire comparison
is dependent on the stream being seekable, since it gets the
length of the streams before starting and Length is undefined
when CanSeek returns false!

Since I didn't feel like putting much work into it right now,
I put in a throw (argumentexception) if either stream is not
readable or seekable. Comparing non-seekable streams or a 
seekable expected to a non-seekable actual might be useful,
so if someone with an interest in this would like to give
it a try, let me know.

BTW, if you write any constraints, it's essential that they
throw rather than returning false when something unexpected
happens. Why? Because somebody can always put "Not." in front
of your constraint, turning the failure into a success.
That's usually not what we want.

Charlie

> -----Original Message-----
> From: [email protected] 
> [mailto:[email protected]] On 
> Behalf Of Charlie Poole
> Sent: Wednesday, August 13, 2008 12:21 PM
> To: 'Gert Driesen'; [email protected]
> Subject: Re: [nunit-developer] StreamsEqual issue(s)
> 
> Hi Gert, 
> 
> > > > 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.
>  
> > > > 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.
> 
> > 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.
> 
> > > > 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. :-)
> 
> > > 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.
> 
> 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.
> 
> Charlie
>  
> > 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=/
> _______________________________________________
> nunit-developer mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/nunit-developer
> 



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