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

Improve ValuesJsonConverter test coverage #108

Conversation

Turnerj
Copy link
Collaborator

@Turnerj Turnerj commented Dec 11, 2019

I've gone through the vast majority of branching logic in ValuesJsonConverter and have written up a number of tests to confirm everything. While a number of these would have been covered by other tests, I have already uncovered a number of strange things in ValuesJsonConverter.

  • Values<> serialization supports writing mixed value types (eg. Values<int, string>) however doesn't support reading it.
  • OneOrMany serialization supports reading single value nullable primitives from JSON however has issues when there is an array of them (the type passed to the constructor is List<int> rather than List<int?>).
  • DateTime reading logic is strange where it can be a string, DateTime, DateTimeOffset and then any other non-primitive type. What I don't understand is how that branch (any other non-primitive to DateTime) is ever hit. Maybe possible if a type implements IConvertible<DateTime> however none of the types in the library do as far as I know.
  • DateTimeOffset has similar strangeness to DateTime though to a greater extent. I don't actually see how value can ever be a DateTimeOffset. My best guess was the MS date format (eg. {"Property":"\/Date(946730040000)\/"} or {"Property":"\/Date(946730040000-0100)\/"}) however in my testing, that is still type DateTime.
  • There is actually an issue with the handling of MS date format having the offset be ignored though that might actually be a JSON.Net issue.
  • The ordering of the generic arguments to Values<> actually matters in relation to the JSON (eg. Values<string, int> with the JSON ["A","B"] throws an exception)

Finally there is a block of code that I don't actually understand when it ever would run:

// REVIEW: If argument still not assigned, only use ToObject if not casting primitive to interface or class
if (args is null)
{
    if (!type.GetTypeInfo().IsInterface && !type.GetTypeInfo().IsClass)
    {
        args = token.ToObject(classType); // This is expected to throw on some case
    }
}

It looks like, after all the other type checks for a value, it is expecting a struct however I actually tried with a struct with constructor initialisation but it doesn't work. Was this meant for struct with settable properties?

I have written tests for some of the issues I've found here though I have marked them as "Skip" - I will activate these tests once I have fixed the individual issues in separate PRs.

@RehanSaeed RehanSaeed merged commit 6c8bf2e into RehanSaeed:master Dec 11, 2019
2 checks passed
@RehanSaeed
Copy link
Owner

RehanSaeed commented Dec 11, 2019

Awesome. Would you like to be added as a contributor of the project?

@Turnerj
Copy link
Collaborator Author

Turnerj commented Dec 11, 2019

Would love that! Would you still like if I opened any changes I have as a PR for you to review?

@Turnerj Turnerj deleted the improve-valuesjsonconverter-test-coverage branch Dec 11, 2019
@RehanSaeed
Copy link
Owner

RehanSaeed commented Dec 11, 2019

I think the PR process is still a good one to make improvements, since nobody writes perfect code. It'll be good for my PR's to also get reviewed.

  • Added you as a contributor to the front page.
  • You should see an invite to be a collaborator.
  • Added to FUNDING.yaml. I don't know why I have this, expect nothing.
  • Added you to the open collective. You should get an invite.

@Turnerj
Copy link
Collaborator Author

Turnerj commented Dec 11, 2019

Yep, no problem! I'll have a couple more PRs coming across the next couple of days. 🙂

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

2 participants