PERL-747: Extended JSON #5
Conversation
…e) due to svtype detection issues
t/lib/CorpusTest.pm
Outdated
| @@ -4,11 +4,13 @@ use warnings; | |||
| use Test::More 0.96; | |||
| use Test::Deep qw/!blessed/; | |||
|
|
|||
| use JSON::PP (); | |||
| use JSON::XS (); | |||
There was a problem hiding this comment.
This sort of defeats the purpose of using JSON::MaybeXS and adds a hard test dependency on JSON::XS, but I don't see it used at all. Is this is leftover from some earlier attempt to fix the ordering problem?
There was a problem hiding this comment.
I've actually now switched it to Cpanel::JSON::XS. The XS/PP usage is because we require PP for the ability to created an IxHash, but we need XS for proper svtype detection. The leftover usage of MaybeXS was from earlier, and I'm now using the XS variant explicitly.
There was a problem hiding this comment.
Can you talk this over with @shadowcat-mst please? Keeping BSON pure-Perl capable -- even in testing -- is pretty important; part of its reason for being, even. If there's no way to avoid XS for testing, then it needs to be conditional.
| $E = _normalize( $E, "$desc: normalizing E" ); | ||
| $cE = _normalize( $cE, "$desc: normalizing cE" ); | ||
|
|
||
| _bson_to_bson( $codec, $B, $cB, "$desc: B->cB" ); |
There was a problem hiding this comment.
It looks like this test and the one on 93 got lost.
There was a problem hiding this comment.
Since the former versions of these used bson and canonical_bson I'm unsure how they would map to the new data fields. I've added a BSON roundtrip test for canonical/converted now, but I'm unsure if this is sufficient.
t/lib/CorpusTest.pm
Outdated
| ->allow_blessed | ||
| ->convert_blessed; | ||
|
|
||
| my $JSON_XS = Cpanel::JSON::XS |
t/lib/CorpusTest.pm
Outdated
| $codec, | ||
| $canonical_bson, | ||
| defined($converted_bson) ? $converted_bson : $canonical_bson, | ||
| 'cB/B -> cB', |
t/lib/CorpusTest.pm
Outdated
| ); | ||
| } | ||
| # deprecated and non-deprecated | ||
| sub _validity_tests_all { |
There was a problem hiding this comment.
I still find these very hard to reconcile against the bson-corpus spec. Could you please try reorganizing to more directly follow the structure in the spec? (e.g. "For cB input...", "For cEJ input..."?
|
Squashed and merged as c863549 |
Implements Extended JSON and adds the new BSON corpus tests.