Skip to content

Enable equality/inequality constraints against non-seekable steams #4483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 8, 2023

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Oct 7, 2023

Fixes #4476

Instead of using the stream's length as a stopping condition, we check that the amount of bytes read in the previous read operation is greater than 0.

@RenderMichael
Copy link
Contributor Author

For the inequality tests, I made the expected and actual streams of the same size. I can double-up each inequality test where the stream lengths aren't equal, let me know if it's worth it.

Same with streams whose contents exceed the BUFFER_SIZE value of 4096. I manually tested against all 4 permutations of the two variables, and they all passed, and I can add tests for it if it's deemed valuable enough.

@stevenaw stevenaw self-assigned this Oct 7, 2023
if (xStream.Length != yStream.Length) return false;
bool bothSeekable = xStream.CanSeek && yStream.CanSeek;

if (bothSeekable && xStream.Length != yStream.Length) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a nice early bailout optimization

Copy link
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RenderMichael this looks great! I've pulled the changes and they work great. I also can't seem to find any edge cases you haven't already accounted for :)

@stevenaw stevenaw merged commit f728164 into nunit:master Oct 8, 2023
@RenderMichael RenderMichael deleted the non-seekable-steams branch October 9, 2023 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to StreamsComparer for non-seekable streams
2 participants