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

Support assertions on System.Text.Json.JsonSerializerOptions based (de)serialization #2246

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

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Aug 1, 2023

An important part of JSON (de)serialization specifications, is defining the intended behavior of it. This (draft) PR an attempt to create a readable/maintainable/extendable syntax, that fits within FluentAssertions.

IMPORTANT

  • If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
  • If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
  • If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
  • If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
    • Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

See: #2205

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Qodana for .NET

16 new problems were found

Inspection name Severity Problems
Possible 'null' assignment to non-nullable entity 🔶 Warning 1
Redundant using directive 🔶 Warning 1
Type member is never used (private accessibility) 🔶 Warning 1
Use preferred style of 'new' expression when created type is not evident ◽️ Notice 10
Arrange null checking pattern ◽️ Notice 2
Use raw string ◽️ Notice 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@Corniel
Copy link
Contributor Author

Corniel commented Aug 23, 2023

Added json.md as a page, to describe the intent.

@Corniel Corniel force-pushed the feature/json-assertions branch 2 times, most recently from ade53f1 to bb67152 Compare August 28, 2023 08:18
/// <param name="becauseArgs">
/// Zero or more objects to format using the placeholders in <see paramref="because" />.
/// </param>
public AndConstraint<JsonElementAssertions> BeString(string value, string because = "", params object[] becauseArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use Be() in every case? The type info on the method name is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that, and if we would only assert on simple nodes, that might would be sufficient.

However, if the JSON is a complex structure, you might want to assert against the JSON string it results in, what is different then the a JSON string node/element. Besides that, it also makes implicit that both the value and the element type are asserted.

In general, what the API should look like for JSON array, and JSON object results, is something I'm still thinking about. I think I personally would use this to assert on the full JSON result as a string:

options.Should().Serialize(myComplexModel)
    And.Shoud().Be("{ 'prop': 'Value', 'other': 12, children: [ ]}");

but others might opt for:

options.Should().Serialize(myComplexModel)
    And.Shoud().Be(new()
    {
        prop = "Value" ,
        other = 12,
        children = Array.Empty<object>()
     });

Where the anonymous object is used to represent the JsonElement. The latter is normally used for BeEquivalent(), but I'm wondering if that has meaning for this kind of assertions.

I'm also wondering if we want the opposite of BeString(), etc. Because, what does that say? That I except something not to be a string? Or a string, but with a different value?!

@dennisdoomen
Copy link
Member

Thanks for taking the lead to spike out the implementation. I still have doubts though.

Consider this example from the PR.

options.Should().Serialize("42").And.Value.Should().BeNumber(42, because: "why not?");

I have two issues with this:

  1. It doesn't follow the same style like the rest of FA's API
  2. And is normally used to execute another assertion on the SUT, which in this case would be options

To make it more idiomatic FA, it could look like this:

"42".Should().BeSerializableToJson(options).Which.Value().Should().BeNumber(42, because: "why not?");

@Corniel
Copy link
Contributor Author

Corniel commented Sep 12, 2023

.Witch, instead of .And is an improvement, I'll change that.

For me, there are two types of (de)serialization tests I'd like to write: when a custom type that is decorated with custom JsonConveter, I would like to test that. In that case the system under test is that custom type, hence: myCustomType.Should(),BeJsonSerializable().Witch.Value().Should().Be("{ 'someProp': 42 }").

The other kind of test, is about the JsonSeralizationOptions defined for an API. In that case, it are the options that are the system under test, and I think that options.Should().Seralize().Which.Value() describe that kind of scenario's better, then the other way around. Actually, in the first case, I Might not want to specify the options in most cases, but use the default settings.

@github-actions
Copy link

github-actions bot commented Sep 13, 2023

Qodana for .NET

10 new problems were found

Inspection name Severity Problems
Type member is never used (private accessibility) 🔶 Warning 1
Use preferred style of 'new' expression when created type is not evident ◽️ Notice 6
Arrange null checking pattern ◽️ Notice 2
Use raw string ◽️ Notice 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@Corniel
Copy link
Contributor Author

Corniel commented Sep 13, 2023

@dennisdoomen About .Should().JsonDeserialize(). would you like to have that as an extension method, or just on ObjectAssertions? (And some others).

@dennisdoomen
Copy link
Member

3> The other kind of test, is about the JsonSeralizationOptions defined for an API. In that case, it are the options that are the system under test, and I think that options.Should().Seralize().Which.Value() describe that kind of scenario's better, then the other way around. Actually, in the first case, I Might not want to specify the options in most cases, but use the default settings.

I'm sorry, but I disagree. JsonSeralizationOptions should never be the subject of a test. It's not the options that serialize or deserialize anything, it's the serializer. In other words, those options are what you pass to the serializer.

About .Should().JsonDeserialize(). would you like to have that as an extension method, or just on ObjectAssertions? (And some others).

I think so, just like Should().BeEquivalentTo

@Corniel
Copy link
Contributor Author

Corniel commented Sep 15, 2023

I'm sorry, but I disagree. JsonSeralizationOptions should never be the subject of a test. It's not the options that serialize or deserialize anything, it's the serializer. In other words, those options are what you pass to the serializer.

That is also true. Unfortunately, the JSON Serializer of System.Text.Json is a static method. It are these options that define its behavior, and you want to test if those the setup of your serializer will result in the kind of serialization you like. (With System.Text.Json, you could actually resolve a serializer with - in that case settings - to have something like: serializer.Should().Serialize()).

in outer words: I would like to be able to say: serializer.Should().Serializer(whatever).Which.Value().Should().Be, but System.Text.Json will not allow me to do that. and to me: options.Should().Serialize(whatever).Which().Value().Should().Be is the best next thing for that scenario.

If I want to specify that because of my JsonOptions, null properties are ignored, it is not about the object that is serialized, but about that it is configured that way (in the options). So to me:

options.Should().Serialize().Which.Value().Should().Be("{}");

Is then better than:

new { Name = (string) null }.Should().BeJsonSerializable(options).Which.Value().Should().Be("{}");

I've thought of another name than Serialize, as technically, the options indeed only describe who to serialize, they do not perform it themselves, but could not come up with a name that also makes clear that the continuation with .Which.Value() does not come as a surprise, or are extremely vebose, like options.Should().SpecifySeralizationOf(whatever).

About .Should().JsonDeserialize(). would you like to have that as an extension method, or just on ObjectAssertions? (And some others).

I think so, just like Should().BeEquivalentTo

Than I'll add that one.

@dennisdoomen
Copy link
Member

new { Name = (string) null }.Should().BeJsonSerializable(options).Which.Value().Should().Be("{}");

What's wrong with this one? There's a subject-under-test, there are the options you want to pass in, it's idiomatic FA, and you get to continue with the resulting JObject (or something like that.

@Corniel
Copy link
Contributor Author

Corniel commented Sep 19, 2023

@dennisdoomen About .Should().JsonDeserialize(). would you like to have that as an extension method, or just on ObjectAssertions? (And some others). That is an advantage of defining it on the options: you have all assertions at one place.

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.

None yet

3 participants