-
Notifications
You must be signed in to change notification settings - Fork 87
fix: enforce int64 for large integer values in tests #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enforce int64 for large integer values in tests #120
Conversation
|
|
|
Welcome @arthurbdiniz! |
arthurbdiniz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an amended commit to my fork and for some reason it was not synced here.
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
/remove-lifecycle rotten |
|
NOTE: the upstream https://github.com/go-yaml/yaml was archived april 1st 2025 (yes april first ... https://github.com/go-yaml/yaml#this-project-is-unmaintained) So we don't have much reason to worry about reconciling diffs with upstream. /lgtm |
|
/approve |
As peer this message for some reason the changes on my forked branch did not sync automatically. So I had sync it. Sorry for dropping the approval. |
|
The "bug" occurs on yaml/go-yaml too GOARCH=386 go test ./...
# go.yaml.in/yaml/v4_test [go.yaml.in/yaml/v4.test]
./decode_test.go:247:25: cannot use -9223372036854775808 (untyped int constant) as int value in map literal (overflows)So I opened |
|
@ccoVeille we recently ran across a similar problem in otel (not exactly the same, but similar). we could strengthen our testing similarly perhaps?
|
|
I feel like this comment was for @arthurbdiniz He owns the current PR. |
|
Apologies. Misunderstood. @ccoVeille has opened this as yaml/go-yaml#129 |
53ce8bb to
a90de8f
Compare
a90de8f to
ef34267
Compare
|
I think instead of changing the interface to func intOrInt64(v int64) interface{} {
// On 64-bit systems, large numbers will be int
// On 32-bit systems, they will remain int64
if strconv.IntSize == 64 {
return int(v)
}
return v
}Here is the reference for the full patch. 0001-Fix-int64-minimum-value-handling-in-tests-for-cross-.patch Now that goyaml.v2 and goyaml.v3 folders are just a reference to go.yaml.in/yaml/* that function can be incorporated at yaml/go-yaml#129 if make sense. cc @ccoVeille |
|
Some yaml_test.go test are still failing so I updated this PR to fix that. |
I'm hesitant with this. I would prefer a type that is defined by arch with build tags, but it would be overkill. Your solution works, but honestly I'm unsure there is a need for it. Your solution to use int64 everywhere sounds a sane and simpler approach. So I'm fine with it. |
Update test cases with overflow errors on 32-bit (i386) architectures. Signed-off-by: Arthur Diniz <[email protected]>
ef34267 to
909bfc4
Compare
liggitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arthurbdiniz, ccoVeille, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The bug arises from the use of generic integer types
(int)in test cases involving large numerical values that exceed the range of a32-bitinteger.In Go, the size of the int type depends on the platform, this behavior causes inconsistencies when handling large integers, such as
-9223372036854775808and9007199254740993.On 32-bit systems, these values cannot be represented correctly as int, leading to potential overflow or truncation errors. By explicitly using the
int64type in the affected test cases, the code ensures proper handling and representation of large integers across all platforms, avoiding platform-specific bugs and improving the reliability of the tests.Reproduce
Run tests setting
GOARCHto386.Changes:
map[string]interface{}tomap[string]int64to ensure type consistency and avoid type mismatch failures in DeepEquals.
-9223372036854775808is treated safely as anint64toprevent compile-time constant overflow on 32-bit platforms.
int64when workingwith large integer constants that exceed int32 range.
These changes address potential issues with integer overflow and ensure compatibility across platforms with differing integer size representations.