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

Figure out serialization support #1499

Merged

Conversation

blairconrad
Copy link
Member

As an outgrowth of recent discussions in #1498, I thought it would be nice to figure out what support we have for serialization, so I consolidated some tests and added a few (and removed one). I think this will help us reason about how to proceed with serialization support.

@blairconrad blairconrad force-pushed the characterize-serializability branch 2 times, most recently from fd96000 to 600962d Compare November 20, 2018 02:15
<PropertyGroup Condition="'$(TargetFramework)' == 'net461'">
<DefineConstants>$(DefineConstants);FEATURE_BINARY_SERIALIZATION</DefineConstants>
</PropertyGroup>

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess removing this isn't required. It's just that we don't use it anymore, so I figured I might as well clean up.

@@ -17,12 +17,13 @@
<ProjectReference Include="..\..\tests\FakeItEasy.Tests\FakeItEasy.Tests.csproj" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp2.1'">
<Compile Remove="SerializationSpecs.cs" />
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not married to using Remove instead of relying on FEATURE_BINARY_SERIALIZATION, so I'm happy to restore the constant if you like. It was just a thing to try out.

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me

// Assert
collection.Should().BeBinarySerializable();
}
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

The only test (except for the weird ones that tested the only-in-the-test-project-DummyException) that I didn't take to the specs. Because once I saw that we can't serialize a fake as soon as it's been called, there didn't seem to be much point in checking that we could serialize a collection of arguments.

thomaslevesque
thomaslevesque previously approved these changes Nov 20, 2018
Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

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

LGTM! Waiting for your answer in #1498 before merging

@@ -17,12 +17,13 @@
<ProjectReference Include="..\..\tests\FakeItEasy.Tests\FakeItEasy.Tests.csproj" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp2.1'">
<Compile Remove="SerializationSpecs.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

Fine by me

Copy link
Member Author

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

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

Updated the test we talked about and rebased.

tests/FakeItEasy.Specs/SerializationSpecs.cs Show resolved Hide resolved
@thomaslevesque thomaslevesque merged commit 34316c4 into FakeItEasy:develop Nov 20, 2018
@thomaslevesque
Copy link
Member

Thanks, @blairconrad!

@blairconrad blairconrad added this to the vNext milestone Nov 20, 2018
@blairconrad blairconrad deleted the characterize-serializability branch November 21, 2018 11:34
@thomaslevesque
Copy link
Member

This change has been released in FakeItEasy 5.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants