Improve Utf8JsonWriter code coverage #36811
Conversation
|
|
madmir
force-pushed
the
improve-Utf8JsonWriter-code-coverage
branch
2 times, most recently
from
ae8f06a to
31f4872
Compare
* Added test for expected argument exception when property name or value arguments reach MaxTokenSize
madmir
force-pushed
the
improve-Utf8JsonWriter-code-coverage
branch
from
31f4872 to
17b4ab5
Compare
|
API changes made my previous commit (testing buffer too small and WriteComment methods) obsolete/redundant. Therefore I got rid of it and the latest test is based on how WriteLargeKeyOrValue and WriteLargeKeyValue are already working. It should increase System.Text.Json.ThrowHelper coverage. |
Can you share the code coverage report? Go to the test directory and run: https://github.com/dotnet/corefx/blob/master/Documentation/building/code-coverage.md |
|
| [InlineData(false, true)] | ||
| [InlineData(true, false)] | ||
| [InlineData(false, false)] | ||
| public void WriteTooLargeArguments(bool formatted, bool skipValidation) |
There was a problem hiding this comment.
This test should run relatively quickly, correct, given the check happens right up front?
I am fine it running in outerloop (given it allocates quite a bit), but just checking that runtime wise it wouldn't cause a significant time cost.
There was a problem hiding this comment.
Yes. Memory allocation is the reason, following example of existing tests with similar setup.
ahsonkhan
left a comment
ahsonkhan
left a comment
There was a problem hiding this comment.
Thanks for the improved test coverage. Looks like the two missing branches in ValidatePropertyNameAndDepth get covered here (previously they were only partially covered):
|
Thanks for your time and giving opportunities for people to get involved. |
Absolutely. We definitely welcome and encourage community contributions whether in the JSON space or anything in this repo (it's open source after all). Feel free to ping me on other PRs of yours and I can help review them or help where possible. There are quite a few up-for-grabs issues: https://github.com/dotnet/corefx/issues?q=is%3Aopen+is%3Aissue+label%3Aup-for-grabs and a few marked as easy as well: https://github.com/dotnet/corefx/issues?q=is%3Aopen+is%3Aissue+label%3Aup-for-grabs+label%3Aeasy Specific to the issue you were working on, we still have some test gaps to fill :) |
|
Seconded @madmir, we invite you to be a regular contributor! |
* Added test for expected argument exception when property name or value arguments reach MaxTokenSize Commit migrated from dotnet/corefx@b35cd0d
4 participants
Uh oh!
There was an error while loading. Please reload this page.