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

refactor: split project in wrapper and abstraction #905

Conversation

vbreuss
Copy link
Member

@vbreuss vbreuss commented Nov 4, 2022

As suggested in this comment split the "System.IO.Abstractions" project into two projects:

  • TestableIO.System.IO.Abstractions
    which contains just the interfaces
  • TestableIO.System.IO.Abstractions.Wrappers
    which contains the wrapper implementations and uses the interface project

System.IO.Abstractions is kept as an empty project which references TestableIO.System.IO.Abstractions.Wrappers and used for backward compatibility

Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @vbreuss!

@rcdailey
Copy link
Contributor

I'm really confused about this change. I'm getting tons of errors:

image

And the dependencies are confusing as well:

image

It seems like the goal was to add the TestableIO prefix, but:

  • The TestingHelpers package does not have a version with that prefix
  • There's cross-contamination between the System.IO ones and the TestableIO.System.IO ones (see the implicitly installed packages in the prior screenshot).
  • The main README.md does not state anything about the new packages, so I can only assume that means we should ignore them. But I get the errors above while building.

@fgreinacher
Copy link
Contributor

@vbreuss Can you support here? 🙇‍♂️

@vbreuss
Copy link
Member Author

vbreuss commented Nov 24, 2022

@vbreuss Can you support here? 🙇‍♂️

I will have a look at it tomorrow!

@vbreuss vbreuss deleted the topic/vb/split-project-in-wrapper-and-abstraction branch November 25, 2022 07:08
@vbreuss vbreuss restored the topic/vb/split-project-in-wrapper-and-abstraction branch November 25, 2022 07:08
@vbreuss
Copy link
Member Author

vbreuss commented Nov 25, 2022

@rcdailey :
I have a hard time reproducing your issue. I created a new test project, which references the "System.IO.Abstractions" packages:


	<PropertyGroup>
		<TargetFrameworks>net472;net6</TargetFrameworks>
	</PropertyGroup>

	<ItemGroup>
		<PackageReference Include="System.IO.Abstractions" Version="17.2.26" />
		<PackageReference Include="System.IO.Abstractions.TestingHelpers" Version="17.2.26" />
	</ItemGroup>

	<ItemGroup>
		<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.1.0" />
		<PackageReference Include="xunit" Version="2.4.1" />
		<PackageReference Include="xunit.runner.visualstudio" Version="2.4.3">
			<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
			<PrivateAssets>all</PrivateAssets>
		</PackageReference>
		<PackageReference Include="coverlet.collector" Version="3.1.2">
			<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
			<PrivateAssets>all</PrivateAssets>
		</PackageReference>
	</ItemGroup>

</Project>

and used a simple unit test that references the IFileInfo you mentioned:

		[Fact]
		public void Test1()
		{
			IFileSystem fileSystem = new MockFileSystem();

			IFileInfo fileInfo = fileSystem.FileInfo.New("foo");

			Assert.False(fileInfo.Exists);
		}

It runs as expected and also the dependencies look fine to me:
image

What did I miss?
Can you please help me reproducing the errors, you are encountering?

@rcdailey
Copy link
Contributor

rcdailey commented Nov 25, 2022

I'm sorry for the complication. I thought this might be straightforward, but apparently not...

My project is open source. Testing there might be the best option, since a simple MCVE doesn't seem to reproduce the issue. It's possible I'm doing something wrong.

My project is here, along with the branch I'm testing the upgrade on: https://github.com/recyclarr/recyclarr/tree/system-io-abstractions-upgrade

Steps to reproduce:

  1. Clone the above repo
  2. git checkout system-io-abstractions-upgrade
  3. cd src
  4. dotnet build

When I attempted to test this again, I noticed a new issue that wasn't there before. I'm not sure why it happens. When I do dotnet restore on this branch, it tells me:

error NU1202: Package TestableIO.System.IO.Abstractions 17.2.26 is not compatible with net7.0 (.NETCoreApp,Version=v7.0). Package TestableIO.System.IO.A
bstractions 17.2.26 does not support any target frameworks.

I think we should analyze this first and see what is going on. If you do the same dotnet restore or even dotnet build on master, you'll see it builds just fine. On master I'm using version 17.2.3.

@vbreuss
Copy link
Member Author

vbreuss commented Nov 26, 2022

Thanks @rcdailey for the explanation and the repo!

I think it relates to the TestableIO.System.IO.Abstractions.Extensions reference.
I could successfully compile your project, when I remove this reference (and add the extension methods manually).

I could also reproduce the same error in my test project:
As soon as I used an extension method from the Extensions project, I stumbled on the same error:
image

@fgreinacher : Can we create a new version of TestableIO.System.IO.Abstractions.Extensions, which references the interface project?
It this project still active (it references version "13.*" of System.IO.Abstractions which is quite old)?

@fgreinacher
Copy link
Contributor

fgreinacher commented Nov 26, 2022 via email

@gigi81
Copy link

gigi81 commented Nov 26, 2022

Will have a look later today!

@rcdailey
Copy link
Contributor

I appreciate everyone looking into this. One last item I think would be nice: Documentation. It's unclear to me which packages I should be using. If it's just a matter of prepending TestableIO. to each of the "legacy" package names, that's simple enough, but the mixture of names right now is a tad bit disorienting. I think a blurb in the readme to talk about the right packages to use, which are legacy / deprecated, etc would be very helpful.

Thanks again everyone!

@vbreuss
Copy link
Member Author

vbreuss commented Nov 27, 2022

@fgreinacher : wouldn't it be better to apply the same change on the TestingHelpers project?
This way, we would have changed all projects to have a "TestableIO" prefix, but keep the existing packages as Mera-Packages around...

I think it would make the package naming more consistent! What do you think?

@fgreinacher
Copy link
Contributor

Thanks for the feedback @rcdailey, appreciated! I have actually expected that this change is transparent to users, but I think there are a few cases like yours where this is actually a breaking change and we should have bumped the major version before releasing it.

I therefore marked 17.2.26 as deprecated on NuGet.org and will prepare an PR for publishing v18.

wouldn't it be better to apply the same change on the TestingHelpers project?
This way, we would have changed all projects to have a "TestableIO" prefix, but keep the existing packages as Mera-Packages around...

Yes, full ack! I will do that as part of the PR!

@rcdailey
Copy link
Contributor

rcdailey commented Dec 4, 2022

Just wanted to write back and let you all know everything is working great now. I upgraded to all the latest as of today and no more issues!

Thank you all for the quick turnaround!

@fgreinacher
Copy link
Contributor

Just wanted to write back and let you all know everything is working great now. I upgraded to all the latest as of today and no more issues!

Thank you all for the quick turnaround!

Cool, thanks for the feedback!

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.

5 participants