Added ShouldBeJsonSerializable#52
Added ShouldBeJsonSerializable#52dennisdoomen merged 6 commits intofluentassertions:masterfrom rikrak:ShouldBeJsonSerializable
Conversation
dennisdoomen
left a comment
dennisdoomen
left a comment
There was a problem hiding this comment.
Apologies, but I noticed I wasn't finished with the review.
| { | ||
| var serializedObject = JsonConvert.SerializeObject(subject); | ||
| var cloneUsingJsonSerializer = JsonConvert.DeserializeObject(serializedObject, subject.GetType()); | ||
| return cloneUsingJsonSerializer; ; |
There was a problem hiding this comment.
🙃 Nitpick: unnecessary semi-colon
| .IncludingFields() | ||
| .IncludingProperties(); | ||
|
|
||
| var typedSubject = (T)assertions.Subject; |
There was a problem hiding this comment.
🤔 What if the subject is not of type T?
🤔 What if the subject is null?
There was a problem hiding this comment.
With respect to what to do if the subject is null: Both BeBinarySerializable() and BeDataContractSerializable() fail the assertion with what looks like a NullReferenceException. I could do the same, but I was thinking of something more like this:
Execute.Assertion.ForCondition(assertions.Subject != null)
.BecauseOf(because, becauseArgs)
.FailWith("Expected {object:context} to be JSON serializable{reason}, but the value is null. Please provide a value for the assertion.");
Not quite sure on the fail message, but it gets the point across I think
There was a problem hiding this comment.
Yeah, that's sloppy old code. The main library will do exactly what you suggested.
| Action act = () => target.Should().BeJsonSerializable(); | ||
|
|
||
| // assert | ||
| act.Should().Throw<Xunit.Sdk.XunitException>(); |
There was a problem hiding this comment.
🤔 I think we should also verify a part of the message to make sure the test fails for the right reasons. Same for the other tests where you expect an exception.
| } | ||
|
|
||
| [Fact] | ||
| public void Class_is_simple_poco_should_be_serializable() |
There was a problem hiding this comment.
🤔 I don't quite get this naming convention in all of these test cases. For this one, did you intend to say "Class as simple poco should be serializable"? Maybe "Simple_poco_should_be_serializable" or "When_class_is_simple_poco_it_should_be_serializable"?
…nition. Also improved test validation to include checks on the fail message
|
I'm not too familiar with the etiquette of github - Should I mark the review comments as Resolved after a commit, or is that up to the reviewer? Either way I've just (hopefully!) addressed the review comments with the latest commits - let me know if they're OK, or if there are any other bits to look at :-) |
|
Thank you for your contribution. |
|
Crap. Completely forgot to ask @jnyrup to also take a peek... |
jnyrup
left a comment
jnyrup
left a comment
There was a problem hiding this comment.
I was thinking out giving this a view later today.
Aside from the single question about inheritance, it looks good to me.
| public static class ObjectAssertionsExtensions | ||
| { | ||
| /// <summary> | ||
| /// Asserts that an object can be serialized and deserialized using the JSON serializer and that it stills retains |
There was a problem hiding this comment.
"stills retains" -> "still retains"
| .Which.Message | ||
| .Should().Contain("value is null") | ||
| .And.Contain("Please provide a value for the assertion"); |
There was a problem hiding this comment.
Tip, it should be possible to shorten this to:
.WithMessage("*value is null*Please provide a value for the assertion*");| private static object CreateCloneUsingJsonSerializer(object subject) | ||
| { | ||
| var serializedObject = JsonConvert.SerializeObject(subject); | ||
| var cloneUsingJsonSerializer = JsonConvert.DeserializeObject(serializedObject, subject.GetType()); | ||
| return cloneUsingJsonSerializer; | ||
| } |
There was a problem hiding this comment.
Does it make any difference if we use subject.GetType() or typeof(T)?
E.g. for inheritance and interfaces.
There was a problem hiding this comment.
Looks like using typeof(T) causes issues with the non-generic overload of BeJsonSerialisable as T is object which causes problems with the equivalency of the deserialised object. Essentially a dynamic object is deserialised that doesn't play well with the IsEquivalentTo test. In addition the deserialised dynamic object has only string properties rather than, for example, Guids or DateTimes that the original instance had.
3 participants
Added ShouldBeJsonSerializable (see #3 )
Note: I've added the extension to the root namespace (FluentAssertions) to aid discovery.
This is based on a previous PR[1] by @borismod that was never merged
[1] 54432fd