feat: Ensure compatibility with encoding/json/v2 experiment

[munlicode]

Closes #4026

Summary

Following the investigation of Go 1.25 support for encoding/json/v2, this PR makes the test suite compatible with the new implementation. While the core library is already compatible, several tests were "brittle"—they relied on exact Go v1 error strings or specific unsupported types that have evolved in v2.

Changes

• Relaxed Error Assertions: Updated error checks in security_advisories_test.go and repos_hooks_deliveries_test.go to use strings.Contains for logical error substrings. This avoids failures due to v2's descriptive field paths (e.g., adding .0 to indexed errors).
• Stable Negative Testing: Refactored TestNewRequest_invalidJSON in github_test.go to use a func() payload. jsonv2 now supports marshaling map[any]any, so using a function ensures the test remains a valid failure test in both JSON versions.
• Future-Proofing: Simplified internal error type checks (errors.As) to make tests more resilient to changes in Go's internal error wrapping or reporting.

Performance Analysis (Benchmarked on Go 1.25.7)

Benchmarks were run on NewRequest comparing the two implementations:

Metric	v1 (Standard)	v2 (Experimental)
Time per Op	3524 ns/op	4896 ns/op
Memory per Op	1560 B/op	2048 B/op
Allocations	20 allocs/op	26 allocs/op

Note: v2 currently shows slightly higher latency and memory usage.

Verification

Tests pass in both environments on Go 1.25.7:

• script/test.sh -> PASS
• GOEXPERIMENT=jsonv2 script/test.sh -> PASS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.07%. Comparing base (c02c318) to head (d917f16).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4029   +/-   ##
=======================================
  Coverage   94.07%   94.07%           
=======================================
  Files         207      207           
  Lines       19163    19163           
=======================================
  Hits        18028    18028           
  Misses        936      936           
  Partials      199      199           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

Thank you, @munlicode!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra

[merchantmoh-debug]

This is a really solid, proactive update. Getting ahead of the Go 1.25 encoding/json/v2 experiment will definitely save us some headaches down the line.

Looking through the diff, a couple of things stand out. First, swapping map[any]any for func() in github_test.go is a super smart catch. Since v2 is way more flexible and can actually marshal map[any]any under the right conditions, using a function is basically a bulletproof way to guarantee an encoding failure across all JSON versions.

Second, shifting to strings.Contains() for the error assertions (like in security_advisories_test.go and repos_hooks_deliveries_test.go) is just good testing hygiene. Hardcoding exact stdlib error strings is a classic anti-pattern since the internal formatting shifts around all the time. Good use of short-circuiting there too (err == nil || !strings.Contains(...)) so we don't accidentally panic on a nil error.

I do have one tiny nitpick—completely non-blocking. In github/github_test.go, you yanked the type assertion check entirely. As it stands now, it just asserts that some error occurred. It's highly unlikely to fail for another reason here (since "GET" and "." are perfectly valid for NewRequest), but dropping the validation completely makes the test feel a little loose.

To keep it resilient but still focused, maybe just swap it for a quick string check? Something like: if err == nil { t.Error("Expected error to be returned.") } else if !strings.Contains(err.Error(), "unsupported") && !strings.Contains(err.Error(), "json") { t.Errorf("Expected a JSON marshaling error; got %v", err) }

Honestly though, it's a minor nit and not worth stalling the PR over unless we want to be hyper-strict on the assertions.

Overall—changes are clean, strictly confined to the test files so there's zero prod risk, and the CI scripts are perfectly green. Since gmlewis already gave the initial approval, I'm good with this. LGTM!

[stevehipwell]

LGTM

[alexandear]

Thank you for investigating the migration to encoding/json/v2. Since the required changes affect only tests, I suggest postponing them for now. json/v2 is expected to reach a stable state in about half a year. Given the compatibility promise, we will likely upgrade to json/v2 no earlier than a year from now.

With that in mind, would it make more sense to apply these changes during the actual upgrade rather than in advance?

For now, we can simply document our findings in the issue and close the PR.

[alexandear]

This is a really solid, proactive update. Getting ahead of the Go 1.25 encoding/json/v2 experiment will definitely save us some headaches down the line.

@merchantmoh-debug, could you please avoid posting AI-generated or low-effort content in this thread?

[gmlewis]

With that in mind, would it make more sense to apply these changes during the actual upgrade rather than in advance?

I'm not seeing any harm in this PR, and I really do like the change from map[any]any to func as that makes a lot of sense to me.
So I'm leaning toward going ahead with this PR even though encoding/json/v2 is a year or more in the future.

@merchantmoh-debug, could you please avoid posting AI-generated or low-effort content in this thread?

I understand your perspective, @alexandear - I really do. However, in my mind, these posts are very similar in spirit to auto-generated GitHub Copilot code reviews... and yet I'm now finding them interesting to at least skim over and sometimes to actually fully read. 😅 If the volume simply becomes too much so that there is more noise than content, then I think we may have to address this in the future... but I honestly found this one interesting to read and informative...

And actually, I was also myself very concerned that we were REMOVING checks in one of the tests in this PR... which I'm still honestly a bit unsure about... and before the post by @merchantmoh-debug, I was ALSO thinking that we should NOT be removing an existing check... and I'm kinda leaning that way right now before I review...

So anyway, my comment is turning out longer than the post above (sorry!). But the bottom line I think is that times are changing and that @merchantmoh-debug hasn't done anything wrong, and I found the post above to be an interesting and informative read, so maybe we should be more tolerant and slower to react. Just my $0.02.

[gmlewis]

OK, after reviewing again, I see that the func() test is truly getting covered at lines 579-581, so I'm happy with this PR.
Still LGTM.

Thank you, again, @munlicode, @stevehipwell, @merchantmoh-debug, and @alexandear!
I do appreciate all of you and your input.

Merging.