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

Can we make SerializationHelper public #2770

Closed
kabua opened this issue Sep 2, 2023 · 3 comments
Closed

Can we make SerializationHelper public #2770

kabua opened this issue Sep 2, 2023 · 3 comments

Comments

@kabua
Copy link

kabua commented Sep 2, 2023

We're writing our own Discoverer classes and trying to follow the framework's pattern, but we've discovered on a few occasions that we need access to the SerializationHelper class but can't because its access level isn't explicitly defined; thus, it defaults to internal. Can this class be made public?

Thanks.

@bradwilson
Copy link
Member

bradwilson commented Sep 3, 2023

@kabua Looking at this, it's a bit problematic. The design of this class isn't great for making public as-is.

It supports serializing things which implement IXunitSerializable, or arrays of generally serializable things. It does not, however, support serializing individual instances of generally serializable things. This limited design is based on where we use it internally: (a) to serialize test case objects, and (b) to serialize theory data (more generally, "test method arguments", which for most use cases means theory data). In addition, the IsSerializable method is just outright incorrect, but I would remove/fix this before making the class public.

I presume you'd be interested in more than just that.

The more versatile class would be XunitSerializationInfo, though its design is modeled after SerializationInfo and intended to be an implementation detail to support classes implementing IXunitSerializable (which are handed an instance of this type as its interface IXunitSerializationInfo and not the concrete type). It does not have a generalized static Serialize and Deserialize method, because it was never meant to be used in that manner.

The design of SerializationHelper has never been made better, because it serves exactly its purpose and no other.

I'd like to think about how this design might be made better before I commit to it.

How are you planning to use this? Do you need any kind of extensibility? Or does the existing behavior serve your purposes, and you just need a way to do your own serialization? Given that our serialization exists solely to support Visual Studio's Test Explorer and dotnet test test runner, what is the scenario that requires you to enable your own pathway of serialization and de-serialization? Answers to these questions will likely help me understand your needs a bit better. Example code and/or a repro project would probably be extremely helpful here.

Thanks!

P.S. It's worth noting that I have changed the design here entirely for v3, and the class is now public and supported.

@bradwilson
Copy link
Member

SerializationHelper is used on both sides of our process: in the runners via xunit.runner.utility and in the engine via xunit.execuction.*.

Marking this class as public on both sides means they end up in different namespaces, to prevent collisions. On the runner side, it lands as Xunit.SerializationHelper, and on the execution side it lands as Xunit.Sdk.SerializationHelper. This is typically how we land types that live on both sides, as they're more likely to be used on the runner side by third parties. For you, since you're making a discoverer, you'll be using the Xunit.Sdk variant.

The code is mostly identical. The salient difference is that, on the execution side, our type name serialization is aware of assemblies decorated with PlatformSpecificAssemblyAttribute and transforms those assembly names appropriately.

The big change here, other than becoming public, is that now all types that we support for serialization are being supported by the Serialize and Deserialize methods (whereas previously we only supported types derived from IXunitSerialization and arrays). IsSerializable remains, and now actually returns the correct information about supported serializable types, since all serialization questions and work are now delegated to XunitSerializationInfo (which remains internal). The only major difference is that null top-level values are not supported by SerializationHelper (that is, you cannot call SerializationHelper.Serialize(null) or SerializationHelper.Deserialize(null)). Of course, null values inside complex types are supported.

@bradwilson
Copy link
Member

Shipped in v2: 2.5.1-pre.33

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

No branches or pull requests

2 participants