-
Notifications
You must be signed in to change notification settings - Fork 234
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
base: master
Are you sure you want to change the base?
Fix/numeric provider state param #506
Conversation
see if we get flakey tests prior to implementing PR with musl support
…at <string, string> this allows for passing of simple string, int and json payloads, rather than simply string
There was a problem hiding this 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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
fixes #449
relates to