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

File system mocking #168

Closed
Tracked by #148
erri120 opened this issue Mar 6, 2023 · 6 comments · Fixed by #176
Closed
Tracked by #148

File system mocking #168

erri120 opened this issue Mar 6, 2023 · 6 comments · Fixed by #176
Assignees

Comments

@erri120
Copy link
Member

erri120 commented Mar 6, 2023

Currently, we use the real file system everywhere. While this makes it easier to implement features, it also makes it harder to write meaningful unit tests. Here are a couple options:

  • Use a virtual file system for testing like System.IO.Abstractions. Since we have custom implementations for various IO related tasks, like dealing with paths, the library might not be a good, and we'd have to use a custom implementation or something else entirely.
  • Keep using the real file system, but provide utility functions and wrappers for test runners to easily create, modify, verify and delete files.

Design

  • Implement an interface for mocking filesystem access. Should accept AbsolutePath types and return our app value types, for example, return Size instead of ulong,
  • Create a default implementation of this IFileSystem interface
  • Add IFileSystem as a parameter type to AbsolutePath.
  • Update AbsolutePath to access the filesystem via IFileSystem. Most of the time we shouldn't need to add a IFilesystem parameter in the rest of the app. If we have an AbsolutePath in a given context we can use the IFileSystem from that path, instead of requiring it as an input to the user-code function.
  • For the remaining usecases we'll need to add IFileSystem as an additional parameter/component
  • Be sure to register the default IFileSystem in the DI framework so we can import it into any component
@erri120 erri120 added the Spike An investigation task label Mar 6, 2023
@erri120
Copy link
Member Author

erri120 commented Mar 6, 2023

In regard to #149, #150 and #151: some form of virtualization or redirection is required for good Wine support. As an example:

public AbsolutePath GetSavesFolder()
{
    return KnownFolders.AppData.CombineUnchecked("SomeGame");
}

With how things are currently, this code will return %APPDATA%\SomeGame on Windows and $XDG_CONFIG_HOME/SomeGame ($XDG_CONFIG_HOME default is ~/.config) on Linux. If we want to support Wineprefxies, we'd need to KnownFolders.AppData to point to the Wineprefix: $WINEPREFIX/drive_c/users/<username>/AppData/Roaming/SomeGame.

This comes with some challenges:

  • we don't know which Wineprefix to use for a specific game at startup (or at static initialization for that matter)
  • the user has to be able to change the Wineprefix as easily as changing what game they want to manage
  • all IO related code should be oblivious to this

The last point is important, so we don't end up having to change everything. A simple drop-in replacement, that doesn't change the code example above, would be ideal. However, since a bunch of utilities, like KnownFolders, are static, we might end up with a big PR.

@Sewer56
Copy link
Member

Sewer56 commented Mar 6, 2023

Not really much to say here; this pretty much lines up with what I've been saying on Slack for a while; so it's nice to have this more immortalised as a comment.

Especially with the last comment. We can't realistically expect developers adding support for individual games to test/write code for non-Windows (sadly), so being able to support non-Windows platforms in a way where plugin/game extension developers are unaware of other OSes would be ideal.

@halgari
Copy link
Collaborator

halgari commented Mar 6, 2023

This all makes sense to me, and I think the hardest part all of this will be the rather massive rewrite of the codebase. But I guess we can do this a bit piecemeal by first adding a method to the filesystem interface, then remove the matching function from AbsolutePath. CombineChecked will also need the filesystem parameter.

We probably want a custom interface for this though, so that we can pass AbsolutePath to it, and not strings like with System.IO.Abstractions

@erri120 erri120 mentioned this issue Mar 7, 2023
10 tasks
@erri120
Copy link
Member Author

erri120 commented Mar 7, 2023

One more caveat in regard to Proton: the default user of Proton-managed Wine prefixes is steamuser, instead of the actual user.

@halgari
Copy link
Collaborator

halgari commented Mar 7, 2023

@erri120 does this sound like something you can tackle? I think we have a pretty solid grip on the design, and I outlined some basic points in the ticket's description

@erri120
Copy link
Member Author

erri120 commented Mar 7, 2023

@erri120 does this sound like something you can tackle? I think we have a pretty solid grip on the design, and I outlined some basic points in the ticket's description

I can start working on this and create a draft PR

@erri120 erri120 added medium and removed Spike An investigation task labels Mar 7, 2023
@halgari halgari added this to MVP Mar 7, 2023
@halgari halgari moved this to In Progress in MVP Mar 7, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in MVP Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants