Streaming JSON parsing cleanup. #115
Conversation
test/json_unittest.cc
Outdated
| )), json::Exception); | ||
| } | ||
|
|
||
| TEST(FromStringTest, Complete) { |
There was a problem hiding this comment.
You can delete TEST(BigTest, RealisticParsing) now
There was a problem hiding this comment.
Heh, so I can. Done.
supriyagarg
left a comment
supriyagarg
left a comment
There was a problem hiding this comment.
Very minor comments.
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(FromStreamTest, Complete) { |
There was a problem hiding this comment.
Complete -> SingleObject for similarity with previous tests?
There was a problem hiding this comment.
FromStream only supports a single object, so that seems a bit redundant. But ok.
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(FromStreamTest, ExpectSingle) { |
There was a problem hiding this comment.
similarly:
ExpectSingle -> MultipleObjectsThrowsError
Makes it clear we are testing similar inputs across multiple methods.
There was a problem hiding this comment.
Makes sense. Done.
test/json_unittest.cc
Outdated
| )), json::Exception); | ||
| } | ||
|
|
||
| TEST(FromStringTest, Complete) { |
There was a problem hiding this comment.
ditto as above:
Complete -> SingleObject
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(FromStringTest, ExpectSingle) { |
There was a problem hiding this comment.
Same as above:
ExpectSingle -> MultipleObjectsThrowsError
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(StreamingTest, ParseStreamThrowsOnParseErrorMidStream) { |
There was a problem hiding this comment.
I find this name (ParseStreamThrowsOnParseErrorMidStream) hard to understand.
Removing the "On" would be better.
There was a problem hiding this comment.
Unfortunately, that would change the meaning I was trying to convey. Renamed — is this better?
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(FromStreamTest, Complete) { |
There was a problem hiding this comment.
FromStream only supports a single object, so that seems a bit redundant. But ok.
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(FromStreamTest, ExpectSingle) { |
There was a problem hiding this comment.
Makes sense. Done.
test/json_unittest.cc
Outdated
| )), json::Exception); | ||
| } | ||
|
|
||
| TEST(FromStringTest, Complete) { |
There was a problem hiding this comment.
Heh, so I can. Done.
test/json_unittest.cc
Outdated
| )), json::Exception); | ||
| } | ||
|
|
||
| TEST(FromStringTest, Complete) { |
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(FromStringTest, ExpectSingle) { |
test/json_unittest.cc
Outdated
| }); | ||
| } | ||
|
|
||
| TEST(StreamingTest, ParseStreamThrowsOnParseErrorMidStream) { |
There was a problem hiding this comment.
Unfortunately, that would change the meaning I was trying to convey. Renamed — is this better?
3 participants
These will make more sense if read one commit at a time.