WIP: fork jsoniter #289
WIP: fork jsoniter #289
Conversation
…iter jsoniter is unmaintained (last commit merged 3 years ago) and has poblematic dependencies (reflect2) this is a stopgap to allow us to patch jsoniter until we can migrate to something like encoding/json/v2 this fork is under internal/ to prevent external usage because we are not committing to keep it long term.
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BenTheElder The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
|
there are a few failing jsoniter tests, this is true even without any real code changes (only cloning and checkout out the release we use, dropping the go package files and updating the import paths) |
|
upstream fixed some of these tests by changing the implementations json-iterator/go@v1.1.12...master there are failing tests even at HEAD / master on a clean clone though: $ pwd
/Users/bentheelder/jsoniter
$ go test ./...
go: downloading github.com/stretchr/testify v1.8.0
ok github.com/json-iterator/go 0.666s
ok github.com/json-iterator/go/any_tests 0.879s
--- FAIL: Test_string_encode_with_std_without_html_escape (0.00s)
encoder_18_test.go:44:
Error Trace: /Users/bentheelder/jsoniter/api_tests/encoder_18_test.go:44
Error: Not equal:
expected: "\"\\b\""
actual : "\"\\u0008\""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-"\b"
+"\u0008"
Test: Test_string_encode_with_std_without_html_escape
FAIL
FAIL github.com/json-iterator/go/api_tests 1.117s
ok github.com/json-iterator/go/benchmarks 0.494s
ok github.com/json-iterator/go/extension_tests 1.388s
ok github.com/json-iterator/go/extra 0.992s
ok github.com/json-iterator/go/misc_tests 0.482s
ok github.com/json-iterator/go/skip_tests 1.240s
ok github.com/json-iterator/go/type_tests 0.904s
--- FAIL: Test_marshal (0.00s)
--- FAIL: Test_marshal/[53]\b/string (0.00s)
value_test.go:77:
Error Trace: /Users/bentheelder/jsoniter/value_tests/value_test.go:77
Error: Not equal:
expected: "\"\\b\""
actual : "\"\\u0008\""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-"\b"
+"\u0008"
Test: Test_marshal/[53]\b/string
--- FAIL: Test_marshal/[57]_/string (0.00s)
value_test.go:77:
Error Trace: /Users/bentheelder/jsoniter/value_tests/value_test.go:77
Error: Not equal:
expected: "\"\\f\""
actual : "\"\\u000c\""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-"\f"
+"\u000c"
Test: Test_marshal/[57]_/string
FAIL
FAIL github.com/json-iterator/go/value_tests 0.324s
FAIL |
probably due to go 1.22 changes - https://go.dev/doc/go1.22
|
|
NOTE: I am aiming to get this into a known-good state before attempting non-trivial modifications and benchmarking those. Fixed the unit test failures. Go vet is failing because of test structs. We can't really fix that because fixing them would defeat the tests, and the go vet command is inline in the prowjob and has no ignore mechanism. Probably we should move vet/apidiff... into hack/ scripts, and we can switch to something like golangci-lint that has ignores. I'll leave that for later. |
|
@BenTheElder: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
The first twelve commits represent just getting the fork into a good state, with minimal code changes: That is until "jsoniter: fix remaining test failure by applying unreleased commit" The next commits contain work on dropping github.com/modern-go/... Later, when that's finished, I'll produce benchmark diffs. |
see: #202
jsoniter is unmaintained, having last merged a commit 3 years ago.
We would like to eliminate the reflect2 dependency.
This PR forks jsoniter into internal/third_party and begins cleaning it up.
TODO: Some additional test-only dependencies are added. I was able to drop one of them so far (gofuzz)
TODO: migrate off of reflect2
TODO: pursue moving to encoding/json/v2 instead
cc @liggitt @jpbetz