Skip to content
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

MockFile.ReadAllLines does not deal with BOM correctly #464

Closed
fuzzykiller opened this issue Apr 15, 2019 · 8 comments · Fixed by #576
Closed

MockFile.ReadAllLines does not deal with BOM correctly #464

fuzzykiller opened this issue Apr 15, 2019 · 8 comments · Fixed by #576
Labels
area: testinghelpers Issues that address the testing helpers state: ready to pick Issues that are ready for being worked on type: bug Issues that describe misbehaving functionality

Comments

@fuzzykiller
Copy link

Or, to be more precise, not at all. To deal with file data, a StreamReader must be used. This is also what .NET internally does.

Consider the following example:

var fs = new MockFileSystem();
fs.AddDirectory("U:\\");
fs.File.WriteAllLines("U:\\Test.txt", new[] { "Hallo Welt"}, Encoding.UTF8);
var chars = fs.File.ReadAllLines("U:\\Test.txt", Encoding.UTF8).Single().ToArray();

chars[0] is now 65279, which is the BOM. That’s because the encoding is used directly:

https://github.com/System-IO-Abstractions/System.IO.Abstractions/blob/452e3312776c14843806b14224ba82ae5d5ec137/System.IO.Abstractions.TestingHelpers/MockFile.cs#L496-L498

I’ll create a pull request later, if that’s okay.


Also, while I’m at it, might I ask why this library even bothers with trying to deal with text as a special case? This introduces unnecessary complexity and potential for differences in behavior with actual System.IO.

@fgreinacher fgreinacher changed the title MockFile.ReadAllLines does deal with BOM correctly MockFile.ReadAllLines does not deal with BOM correctly Apr 19, 2019
@fgreinacher fgreinacher added area: testinghelpers Issues that address the testing helpers state: ready to pick Issues that are ready for being worked on type: bug Issues that describe misbehaving functionality labels Apr 19, 2019
@304NotModified
Copy link
Contributor

@fgreinacher could you please answer these questions?

I’ll create a pull request later, if that’s okay.

Also, while I’m at it, might I ask why this library even bothers with trying to deal with text as a special case? This introduces unnecessary complexity and potential for differences in behavior with actual System.IO.

@fgreinacher
Copy link
Contributor

@fgreinacher could you please answer these questions?

I’ll create a pull request later, if that’s okay.

Sure, that would be fine.

Also, while I’m at it, might I ask why this library even bothers with trying to deal with text as a special case? This introduces unnecessary complexity and potential for differences in behavior with actual System.IO.

TBH, I have no idea. Maybe @tathamoddie remembers?

@304NotModified
Copy link
Contributor

Also, while I’m at it, might I ask why this library even bothers with trying to deal with text as a special case? This introduces unnecessary complexity and potential for differences in behavior with actual System.IO.

I think drop

Maybe include this also for V6?

@fuzzykiller
Copy link
Author

Sorry for the long delay. Upon further investigation I found that the default encoding in MockFileContent does not match the .NET default. This leads to further inconsistencies between this library and the real filesystem. I will not fix this fundamental flaw, it’s too much of a change.

I have a unit test for the reported issue here:

https://github.com/fuzzykiller/System.IO.Abstractions/commit/b525129938cafd6af9cef4059b2ba5d46936c04c

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@siprbaum
Copy link
Contributor

I see that this is marked as stale, but it looks like a real problem.
Due to the the lack of a concrete solution, could this at least be documented as a known issue, e.g. in the release notes or the README?

@fgreinacher
Copy link
Contributor

No worries, issues that get comments won’t be closed, that’s what this workflow is for 😊

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@PeterDowdy PeterDowdy mentioned this issue Apr 13, 2020
fgreinacher added a commit that referenced this issue Apr 17, 2020
This mitigates issues when the file has a BOM. 

Fixes #464

Co-authored-by: Florian Greinacher <fgreinacher@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testinghelpers Issues that address the testing helpers state: ready to pick Issues that are ready for being worked on type: bug Issues that describe misbehaving functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants