Fix int64 minimum value handling in tests for cross-architecture compatibility#129
Fix int64 minimum value handling in tests for cross-architecture compatibility#129ingydotnet merged 3 commits intoyaml:mainfrom
Conversation
936c4f7 to
9e513c3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes cross-architecture compatibility issues in Go YAML tests by addressing integer type handling for large values that exceed 32-bit integer ranges. The changes ensure tests work correctly on both 32-bit and 64-bit platforms by explicitly using int64 types for large integer constants.
- Switched test expectations from
map[string]anyandmap[string]inttomap[string]int64for large integer values - Enhanced CI/CD pipeline to test across multiple architectures including 32-bit systems
- Added proper handling for race detection testing based on architecture constraints
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| decode_test.go | Updated test expectations to use int64 for large integer constants to prevent overflow on 32-bit systems |
| .github/workflows/go.yaml | Expanded CI matrix to test multiple OS/architecture combinations and handle 32-bit specific constraints |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/assign @liggitt |
|
@ccoVeille How are you feeling on this PR? |
For me this PR should be merged, code is fine now. I wish I have a few approvals, but it could be merged as this, I think. |
|
changes lgtm, I'd suggest squashing commits |
|
@ccoVeille the current changes to |
Could you please spot a line of the file that seems out of the PR scope ? The changes I'm introducing are about testing all architectures (os, CPU and 32/64bits) in the unit tests. Something that was missing and caused the issues to be undetected. So I'm fixing the unit tests. Make sure the issues cannot happen again. Maybe I introduced a change that shouldn't be there. Please help to spot the issue if there are any |
864d27c to
9a8512b
Compare
Add multi-platform CI testing for darwin/amd64, darwin/arm64, windows/amd64, linux/386, and windows/386 to ensure go-yaml works correctly across all supported architectures. Signed-off-by: Arthur Diniz <arthurbdiniz@gmail.com> Co-authored-by: Jordan Liggitt <liggitt@google.com>
|
I got this older PR resolved and working again with the current code. Can you give it a quick review, please? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ccoVeille you might want to look it over as well. |
ccoVeille
left a comment
There was a problem hiding this comment.
I'm good with the PR.
Thanks for checking. Given that I'm going to go ahead and merge it. |
The bug arises from the use of generic integer types (int) in test cases involving large numerical values that exceed the range of a 32-bit integer.
In Go, the size of the int type depends on the platform, this behavior causes inconsistencies when handling large integers, such as -9223372036854775808 and 9007199254740993.
On 32-bit systems, these values cannot be represented correctly as int, leading to potential overflow or truncation errors. By explicitly using the int64 type 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 GOARCH to 386.
GOARCH=386 GOOS=linux go test -vet=off -v
Changes:
These changes address potential issues with integer overflow and ensure compatibility across platforms with differing integer size representations.
This PR applies kubernetes-sigs/yaml#120 to from https://github.com/kubernetes-sigs/yaml to yaml/go-yaml
This bug was identified and fixed by @arthurbdiniz
Fixes #128