Fix int64 minimum value handling in tests for cross-architecture compatibility #129
Conversation
ccoVeille
force-pushed
the
i386
branch
5 times, most recently
from
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.
ccoVeille
requested review from
carloslima,
Copilot,
dims,
ingydotnet and
stefanprodan
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ${{ matrix.os }} | ||
| continue-on-error: true # make sure to run all versions, even if one fails | ||
| strategy: | ||
| fail-fast: false # continue to run all jobs even if one fails | ||
| matrix: | ||
| include: | ||
| - { | ||
| os: macos-latest, | ||
| GOOS: darwin, | ||
| GOARCH: amd64, | ||
| test-args: -race | ||
| } | ||
| - { | ||
| os: macos-latest, | ||
| GOOS: darwin, | ||
| GOARCH: arm64, | ||
| test-args: -race | ||
| } | ||
| - { | ||
| os: windows-latest, | ||
| GOOS: windows, | ||
| GOARCH: amd64, | ||
| test-args: -race | ||
| } | ||
| - { | ||
| os: ubuntu-latest, | ||
| GOOS: linux, | ||
| GOARCH: 386, | ||
| test-args: '' # race is not available on 32 bits architecture | ||
| } | ||
| - { | ||
| os: windows-latest, | ||
| GOOS: windows, | ||
| GOARCH: 386, | ||
| test-args: '' # race is not available on 32 bits architecture | ||
| } | ||
| env: | ||
| # GOARCH and GOOS are used by the Go toolchain | ||
| GOARCH: ${{ matrix.GOARCH }} | ||
| GOOS: ${{ matrix.GOOS }} | ||
| name: > | ||
| Build with Go ${{ matrix.go-version }} | ||
| for ${{ matrix.GOOS }}/${{ matrix.GOARCH }} | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - name: Set up Go | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ matrix.go-version }} | ||
| - name: Run go vet | ||
| run: go vet ./... | ||
| - name: Run yaml-test-suite tests | ||
| run: make test-yts | ||
| # make test-yts can only be run on non-Windows OS | ||
| if: ${{ matrix.GOOS != 'windows' }} | ||
| env: | ||
| # This one is used by makeplus/makes | ||
| OS-TYPE: ${{ matrix.GOOS }} | ||
| - name: Run go test | ||
| run: GO111MODULE=on go test -v -race . | ||
| run: go test -v ${{ matrix.test-args }} . |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
|
@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 |
Update test cases that use the int64 minimum value (-9223372036854775808) to avoid overflow errors on 32-bit (i386) architectures. Signed-off-by: Arthur Diniz <[email protected]>
Co-authored-by: Jordan Liggitt <[email protected]>
We now also launch tests on multiple architectures in our CI pipeline. - Linux (amd64, i386) - Windows (amd64, i386) - macOS (amd64, arm64)
|
@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 |
6 participants
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