Skip to content

Conversation

@Turnerj
Copy link
Collaborator

@Turnerj Turnerj commented Dec 16, 2019

When looking into some issues with #100, I realised that the unit tests aren't setup to test .NET Framework. For that PR at least, it shows a large number of issues with .NET Framework - I don't know if that is from the custom ValuesJsonConverter or from System.Text.Json itself.

In either case however, I realised it would be a good thing to have even outside of that PR (for example, testing #109). There is actually one failing test when testing .NET Framework on master:

[Fact]
public void ReadJson_Values_SingleValue_ThingInterfaceWithNoTypeToken()
{
var json = "{\"Property\":" +
"{" +
"\"@context\":\"https://schema.org\"," +
"\"@id\":\"http://example.com/book/1\"," +
"\"name\":\"The Catcher in the Rye\"," +
"\"url\":\"http://www.barnesandnoble.com/store/info/offer/JDSalinger\"," +
"\"author\":{" +
"\"@type\":\"Person\"," +
"\"name\":\"J.D. Salinger\"" +
"}," +
"}" +
"}";
var result = this.DeserializeObject<Values<string, IBook>>(json);
Assert.Empty(result.Value2);
}

The test fails as the current ValuesJsonConverter on master returns null due to not finding a suitable type. This isn't a problem in .NET Core 3 with JSON.Net but is a problem in .NET Framework 4.7.2 - this issue however should already be resolved in #109 due to how it attempts to find a concrete type.

I'm not fully versed in the build process for Schema.NET so I don't know if it is going to be as easy as the two changes I've made. I guess we will see when the CI finishes...

If you are curious, the StringExtensions class is to work around the combination of multiple calls to these specific methods with those methods not being supported in .NET Framework and the linting requiring these calls in .NET Core. I didn't really want to wrap the individual calls in linting suppression statements.

The StringExtensions class is to work around the combination of multiple calls to these specific methods with those methods not being supported in .NET Framework and the linting requiring these calls in .NET Core.
Copy link
Owner

@RehanSaeed RehanSaeed left a comment

Choose a reason for hiding this comment

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

Few questions.

@Turnerj
Copy link
Collaborator Author

Turnerj commented Dec 17, 2019

With the .NET Framework tests running on each platform (with the CI failures coming from a specific test which should be fixed as part of #109), are you happy if we merge it in?

@RehanSaeed
Copy link
Owner

I think it makes sense for us to get #109 in first and come back to this.

@RehanSaeed
Copy link
Owner

This should now build successfully, now that #109 is in.

@RehanSaeed RehanSaeed merged commit af6ef6d into RehanSaeed:master Dec 19, 2019
@Turnerj Turnerj deleted the unit-testing-net-framework branch December 19, 2019 13:07
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.

2 participants