Repository navigation
Add support to StreamsComparer for non-seekable streams #4476
Description
For example, ZipArchiveEntry.Open() opens unseekable steams
[Test]
public void ZipArchiveTest()
{
const string fileContentString = "Hello!";
using var expectedFile = new MemoryStream(Encoding.UTF8.GetBytes(fileContentString));
using var archiveContents = new MemoryStream();
using ZipArchive actualArchive = GetZipArchiveFromSomewhere(archiveContents); // opened in read mode
ZipArchiveEntry entry = actualArchive.Entries[0];
using Stream entryStream = entry.Open(); // an archive in read mode returns a DeflateStream, which is unseekable
Assert.That(entryStream, Is.EqualTo(expectedFile)); // System.ArgumentException : Stream is not seekable (Parameter 'actual')
static ZipArchive GetZipArchiveFromSomewhere(MemoryStream archiveContents)
{
using (var archive = new ZipArchive(archiveContents, ZipArchiveMode.Create, leaveOpen: true))
{
ZipArchiveEntry demoFile = archive.CreateEntry("Hello entry");
using Stream entryStream = demoFile.Open();
using var entryFs = new StreamWriter(entryStream);
entryFs.Write(fileContentString);
}
return new ZipArchive(archiveContents, ZipArchiveMode.Read, leaveOpen: false);
}
}I can work on a PR, let me know if this is something worth pursuing.
edit: improved the repro to be all in-memory
Activity
stevenaw commentedon Oct 3, 2023
Thanks @RenderMichael I like this idea 😄
I think it'll be important to retain the current behaviour over seekable streams which tracks the current position of the stream at the time it's passed in in order to reset the position to 0 so that the whole stream can be compared.
I'm not sure if that difference in behaviour between seekable and non-seekable streams could be a cause for confusion later - but not something that I think is worth factoring in now. If it is a concern later on which needs disambiguation then it could likely be addressed at that time via some sort of "check whole stream" equality modifier.
stevenaw commentedon Oct 3, 2023
@RenderMichael Assigning this your way since you've expressed an interest in it
RenderMichael commentedon Oct 3, 2023
@stevenaw What if only one stream is seekable? Should neither be reset, or only the one?
stevenaw commentedon Oct 4, 2023
Good question, I hadn't thought of that.
My first instinct was to suggest that we only reset the streams if both are seekable (the existing behavior) as I suspect it would be easier to opt into seeking than to opt out in the scenario that only one stream is seekable. Though I feel like that could be "wrong". It could be confusing to only reset a MemoryStream (for example) under certain cases, and resetting a stream is a very nice convenience feature when a test writes to the stream for the purposes of comparison.
@OsirisTerje @manfred-brands what are your thoughts for this scenario? Reset streams in all cases when possible?
RenderMichael commentedon Oct 4, 2023
Would it make sense to surface a property on the equality constraint like
WithoutResettingStreamsso the original behavior is maintained without code changes, but you could opt into non-resetting streams if need be?Maybe it could even disallow non-seekable streams unless that property was set.
stevenaw commentedon Oct 7, 2023
Sorry I'm a few days between replies. I had been thinking of suggesting that too, though that sort of thing is harder to change later once it's in if we target the "wrong" behaviour. Though it's easy to add in after if we find a strong use case where we'd need it.
I also like the idea of keeping this simple to give you a clear path forward - especially if you are looking to make this a Hacktoberfest contribution😄. I'd suggest avoid adding
WithoutResettingStreamsfor now so that we can compare as much of the streams content as we can by resetting any seekable streams by default.RenderMichael commentedon Oct 7, 2023
I opened #4483 for this.
stevenaw commentedon Oct 7, 2023
Thanks @RenderMichael ! I've assigned myself for review on the PR.