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

Fix/numeric provider state param #506

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

YOU54F
Copy link
Member

@YOU54F YOU54F commented Jun 10, 2024

Copy link
Contributor

@adamrodger adamrodger left a comment

Choose a reason for hiding this comment

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

If the API is to open up to allow absolutely anything be used as a provider state parameter and change the public API then I'd want to see more significant testing of "weird" values.

So there are obvious things like what does double.NaN do? What about null or other nullable value types? What about complex objects? What about random classes being passed in? What about string-like types such as StringBuilder and Span<char>? What about arrays or other collection types? Stuff like that.

We also need a lot of round trip testing, to make sure that the provider tests can actually deserialise these things. I know we expect users to handle provider states themselves but we at least need to make sure that System.Text.Json can round trip these things given that's the main serialiser we support.

Using object in a public API creates a lot of opportunity for that to all go wrong so I think we need much more comprehensive testing when we're serialising potentially anything.

@@ -21,7 +21,7 @@ public interface IMessageBuilderV3
/// <param name="providerState">Provider state description</param>
/// <param name="parameters">Provider state parameters</param>
/// <returns>Fluent builder</returns>
IMessageBuilderV3 Given(string providerState, IDictionary<string, string> parameters);
IMessageBuilderV3 Given(string providerState, IDictionary<string, object> parameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is a breaking change to the public API

=> NativeInterop.GivenWithParam(this.interaction, description, name, value).CheckInteropSuccess();
public void GivenWithParam(string description, string name, object value)
{
var jsonValue = System.Text.Json.JsonSerializer.Serialize(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Don't use var when it's not obvious what the type is

style: Don't use fully qualified references inline, use an import instead

@@ -1,7 +1,7 @@
using System;
using System.Runtime.InteropServices;
using PactNet.Interop;

using System.Text.Json.Serialization;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Blank line after import statements

@YOU54F YOU54F marked this pull request as draft September 26, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants