-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Conversation
test/powershell/Modules/Microsoft.PowerShell.Utility/Json.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Json.Tests.ps1
Outdated
Show resolved
Hide resolved
# 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 |
There was a problem hiding this comment.
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?
test/powershell/Modules/Microsoft.PowerShell.Utility/Json.Tests.ps1
Outdated
Show resolved
Hide resolved
If there are other tests you'd like to see please comment and Ill take care of writing them! |
Ive re-added the WIP tag as I am still adding more tests and a couple of the tests are failing at the moment. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
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 |
There was a problem hiding this 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.
test/powershell/Modules/Microsoft.PowerShell.Utility/Json.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Json.Tests.ps1
Outdated
Show resolved
Hide resolved
I've resolved all open comments that had questions or concerns about the code in the PR |
@strawgate Thanks for your contribution! |
🎉 Handy links: |
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
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).