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

Add tests for ConvertFrom-Json #15706

Merged
merged 9 commits into from
Jul 16, 2021
Merged

Conversation

strawgate
Copy link
Contributor

@strawgate strawgate commented Jul 1, 2021

PR Summary

The current json tests do not adequately test datetimes and integers and while there are some unit tests for primitives there are no "large object" tests. I created a little test generator to take an object and produce tests using the various formats.

The datetime tests also do specific things in the case that newtonsoft chooses to convert the json string into a datetime versus when it does not.

The complex object test makes sure that our unit tests apply even when the object is large, with a variety of types and with nested arrays and objects.

The number tests make sure that we properly capture when newtonsoft decides to give us int64 vs double vs bigint objects.

This will make it apparent when changes to the deserializer cause a difference in behavior for datetime , numbers or complex objects.

PR Context

Improve test coverage for json cmdlets. These tests document current behavior and do not have an opinion on what correct behavior looks like. These will help with a future move to system.text.json in understanding where the differences in deserialization are.

PR Checklist

@ghost ghost assigned daxian-dbw Jul 1, 2021
@strawgate strawgate changed the title Add datetime tests to the ConvertFrom-Json tests Add tests for ConvertFrom-Json Jul 1, 2021
Comment on lines 580 to 581
# Counter intuitively all four of these tests pass because precision is lost on both sides of the test, likely due to powershell number handling
ConvertFrom-Json -InputObject 500000000000.0000000000000001 | should -be 500000000000
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vexx32 Have you thoughts/concerns about this?

@iSazonov iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Jul 1, 2021
@strawgate
Copy link
Contributor Author

If there are other tests you'd like to see please comment and Ill take care of writing them!

@strawgate strawgate changed the title Add tests for ConvertFrom-Json WIP: Add tests for ConvertFrom-Json Jul 1, 2021
@strawgate
Copy link
Contributor Author

Ive re-added the WIP tag as I am still adding more tests and a couple of the tests are failing at the moment.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 1, 2021

If there are other tests you'd like to see please comment and Ill take care of writing them!

  1. I'd look existing issues for Json cmdlets to discover scenarios we could cover by tests.
  2. Using null values.
  3. Different enumerations (like array, Dictionary, HashSet and so on)

@ghost ghost added the Review - Needed The PR is being reviewed label Jul 9, 2021
@ghost
Copy link

ghost commented Jul 9, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@strawgate
Copy link
Contributor Author

strawgate commented Jul 9, 2021

If there are other tests you'd like to see please comment and Ill take care of writing them!

  1. I'd look existing issues for Json cmdlets to discover scenarios we could cover by tests.
  2. Using null values.
  3. Different enumerations (like array, Dictionary, HashSet and so on)

I was hoping to focus on the convertfrom-json tests for this one. I think that excludes the enumerations part, i think the existing tests cover null values, and I couldnt find any closed issues where i could add tests. The open issues I saw require code changes to resolve (in addition to adding tests when they are fixed).

I think this is ready for review

@strawgate strawgate changed the title WIP: Add tests for ConvertFrom-Json Add tests for ConvertFrom-Json Jul 9, 2021
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

I think they should be in ConvertFrom-Json.Tests.ps1 file.

@strawgate
Copy link
Contributor Author

I've resolved all open comments that had questions or concerns about the code in the PR

@daxian-dbw daxian-dbw removed the Review - Needed The PR is being reviewed label Jul 16, 2021
@daxian-dbw daxian-dbw merged commit 0e1160d into PowerShell:master Jul 16, 2021
@daxian-dbw daxian-dbw added this to the 7.2.0-preview.8 milestone Jul 16, 2021
@daxian-dbw
Copy link
Member

@strawgate Thanks for your contribution!

@ghost
Copy link

ghost commented Jul 22, 2021

🎉v7.2.0-preview.8 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-Test Indicates that a PR should be marked as a test change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants