Skip to content

Add testing support for .NET Framework #113

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

Merged
merged 7 commits into from
Dec 19, 2019

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