Skip to content
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

ci: also launch tests on i386 #47

Merged
merged 2 commits into from
Nov 15, 2024
Merged

ci: also launch tests on i386 #47

merged 2 commits into from
Nov 15, 2024

Conversation

ccoVeille
Copy link
Owner

Fixes #6
Fixes #46

@ccoVeille ccoVeille force-pushed the i386 branch 2 times, most recently from 4eeb629 to 2adbb05 Compare November 13, 2024 00:34
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1e9618e) to head (d12e3eb).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #47   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          185       185           
=========================================
  Hits           185       185           

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

@ccoVeille
Copy link
Owner Author

It's the first time I'm doing that @ldemailly

what do you think?

@ccoVeille
Copy link
Owner Author

cc @jas4711 for a follow-up on Debian packaging

@ccoVeille
Copy link
Owner Author

It's the first time I'm doing that @ldemailly

what do you think?

Could you also take a look @mmorel-35 @yassinebenaid ?

I'm curious about your feedbacks.

@jas4711
Copy link

jas4711 commented Nov 14, 2024

cc @jas4711 for a follow-up on Debian packaging

Old 1.1.0 is confirmed failing on armhf and i386, both 32-bit archs:

https://ci.debian.net/packages/g/golang-github-ccoveille-go-safecast/testing/armhf/54320394/
https://ci.debian.net/packages/g/golang-github-ccoveille-go-safecast/testing/i386/54320449/

Likely also armel, but that arch is slow and hasn't run the build yet.

I have packaged your i386 branch as a new Debian upload, and it builds fine on i386:

https://salsa.debian.org/jas/golang-github-ccoveille-go-safecast/-/pipelines/762527

I am now uploading this to test on other archs, but hopefully it is fine. I would appreciate a new release eventually!

/Simon

asserters_test.go Outdated Show resolved Hide resolved
examples_32bit_test.go Show resolved Hide resolved
@ccoVeille
Copy link
Owner Author

I would appreciate a new release eventually!

@jas4711 sure, I'll release once merged

@jas4711
Copy link

jas4711 commented Nov 14, 2024

This has some problems even on 64-bit platforms, check test results:

https://tracker.debian.org/pkg/golang-github-ccoveille-go-safecast

This uses dd8172d

Good thing it now works on i386 :)

For example arm64 -- https://ci.debian.net/packages/g/golang-github-ccoveille-go-safecast/testing/arm64/54342388/

 79s === RUN   TestToInt32/from_float64/positive_out_of_range
 79s     asserters_test.go:230: expected error
 79s --- FAIL: TestToInt32 (0.00s)
 79s     --- PASS: TestToInt32/from_int (0.00s)
...
 79s     --- PASS: TestToInt32/from_uint64 (0.00s)
 79s         --- PASS: TestToInt32/from_uint64/zero (0.00s)
 79s         --- PASS: TestToInt32/from_uint64/positive_within_range (0.00s)
 79s         --- PASS: TestToInt32/from_uint64/positive_out_of_range (0.00s)
 79s     --- FAIL: TestToInt32/from_float32 (0.00s)
 79s         --- PASS: TestToInt32/from_float32/zero (0.00s)
 79s         --- PASS: TestToInt32/from_float32/rounded_value (0.00s)
 79s         --- PASS: TestToInt32/from_float32/positive_within_range (0.00s)
 79s         --- FAIL: TestToInt32/from_float32/positive_out_of_range (0.00s)
 79s     --- FAIL: TestToInt32/from_float64 (0.00s)
 79s         --- PASS: TestToInt32/from_float64/zero (0.00s)
 79s         --- PASS: TestToInt32/from_float64/rounded_value (0.00s)
 79s         --- PASS: TestToInt32/from_float64/positive_within_range (0.00s)
 79s         --- FAIL: TestToInt32/from_float64/positive_out_of_range (0.00s)
 79s === RUN   TestToUint32

Could you take a look at the logs? Seems like the float changes introduced some bugs, before (v1.10.0) it passed on all 64-bit platforms.

Fixes #6
Fixes #46

The tests in conversion_test.go are the ones that are not architecture dependent
The tests in conversion_64bit_test.go complete them for 64-bit systems

The tests in examples_test.go are the ones that are not architecture dependent
The tests in examples_32bit_test.go are for 32-bit systems
The tests in examples_64bit_test.go are for 64-bit systems

The architecture dependent file covers the fact, you can reach a higher value with int and uint
on 64-bit systems, but you will get a compile error on 32-bit.

The error message is different on 32-bit and 64-bit systems
The max is 9223372036854775807 on 64-bit and 2147483647 on 32-bit
@ccoVeille
Copy link
Owner Author

@jas4711 I added a new commit to try to fix the issue you are facing with float imprecision

d12e3eb

Could you tell me if if works ?

@jas4711
Copy link

jas4711 commented Nov 15, 2024

@jas4711 I added a new commit to try to fix the issue you are facing with float imprecision

d12e3eb

Could you tell me if if works ?

I've uploaded 1.1.0+git20241114-1 to Debian and results should arrive here within the next day or so:

https://tracker.debian.org/pkg/golang-github-ccoveille-go-safecast

Please ignore the results for 20241113, they are for the old version.

@jas4711
Copy link

jas4711 commented Nov 15, 2024

The build looks good now. A new release would be great! Thank you.

Copy link

@yassinebenaid yassinebenaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔅

@ccoVeille ccoVeille merged commit a592ff0 into main Nov 15, 2024
5 checks passed
@ccoVeille ccoVeille deleted the i386 branch November 15, 2024 21:08
@ccoVeille
Copy link
Owner Author

The build looks good now. A new release would be great! Thank you.

@jas4711 1.2.0 is out

@jas4711
Copy link

jas4711 commented Nov 19, 2024

FWIW, 1.2.0 has now been built fine on all architectures, so I can confirm that this is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for 32-bit architectures? Make sure code can be run on 32 bits version
4 participants