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

Add new test for array_fill() to cover the case when the parameter count is too large #11184

Merged
merged 1 commit into from May 5, 2023

Conversation

jquiaios
Copy link
Contributor

@jquiaios jquiaios commented May 3, 2023

I added a new test to cover an untested case in the array_fill() function.

Before
image

After
image

@iluuu1994
Copy link
Member

@jquiaios Thank you for your contribution! Not sure if we really need this test. Tests that use huge amounts of memory can influence other tests that run in parallel in CI, our CI is already very flaky. On 32-bit your test fails because int to float promotion happens on "INT32_MAX" + 1.

@jquiaios
Copy link
Contributor Author

jquiaios commented May 3, 2023

@mvorisek @iluuu1994 Thank you both for your feedback. What do you recommend then? Should I try to divide this test into two, one testing the x32 and the other x64? Or just forget about this test to avoid making the CI flakier ?

Also if you have any recommendation about other functionalities to test instead of these kinds of very edge cases, please let me know, I'm new to contributing and any advice to improve the experience is good to take.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Right, on 32-bit this error can't occur because INT32_MAX + 1 overflows to float and leads to a type error. Thus the test needs to be skipped for 32-bit. This can be done with if (PHP_INT_SIZE != 8) die("skip: 64-bit only"); in the SKIPIF section.

The test is actually fine, it doesn't allocate any memory, it fails before doing so. So this should impact other tests. 👍

ext/standard/tests/array/array_fill_error2.phpt Outdated Show resolved Hide resolved
ext/standard/tests/array/array_fill_error2.phpt Outdated Show resolved Hide resolved
@jquiaios jquiaios force-pushed the master branch 2 times, most recently from adf7cad to e81d594 Compare May 3, 2023 22:40
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Great, thank you @jquiaios!

@Girgias Girgias merged commit bb38ad7 into php:master May 5, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants