Skip to content

Add other test cases for bcadd and bccomp #4110

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Add other test cases for bcadd and bccomp #4110

wants to merge 3 commits into from

Conversation

peter279k
Copy link
Contributor

@peter279k peter279k commented May 3, 2019 •
edited
Loading

Changed log

  • Add other test cases for bcadd and bccomp function variations.

@nikic
Copy link
Member

nikic commented May 7, 2019

The error case tests in here will probably be superseded by #4092.

@peter279k
Copy link
Contributor Author

The error case tests in here will probably be superseded by #4092.

@nikic, should I add other error test case from PR #4092?

@cmb69
Copy link
Member

cmb69 commented Jun 2, 2019

@peter279k Please rebase this PR onto current PHP-7.4. Likely some of these test will fail then, so please adjust them. Thanks!

@peter279k
Copy link
Contributor Author

Hi @cmb69, thank you for your reply.

I've merged branch PHP-7.4 to this PR.

@peter279k peter279k force-pushed the enhance_bcadd_bccomp branch from f149a1c to 3866848 Compare June 5, 2019 05:11
cmb69
cmb69 reviewed Jun 5, 2019
View reviewed changes
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Looks generally good to me. Not sure if we need all these tests for non-numeric values, though.

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2021

After consideration, I don't think the new tests make much sense; after all, checking for non-wellformed arguments is already done elsewhere. The additions to the existing test cases are fine, but I don't see a good reason to merge that into a stable branch.

So, @peter279k, could you please rebase onto master, and change the target branch accordingly?

@peter279k peter279k mentioned this pull request Dec 29, 2021
Merged
@peter279k
Copy link
Contributor Author

Closing this and reopen on PR #7851.

@peter279k peter279k closed this Dec 29, 2021
@peter279k peter279k deleted the enhance_bcadd_bccomp branch December 29, 2021 09:43
peter279k added a commit to peter279k/php-src that referenced this pull request Dec 30, 2021
@peter279k
For PR php#4110 to diff for master branch
6af5924
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 left review comments

Assignees
No one assigned
Labels
Category: Tests Waiting on Author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@peter279k @nikic @cmb69 @carusogabriel