Skip to content

mbstring: Do not stop when mbstring test faild #10009

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

Conversation

youkidearitai
Copy link
Contributor

I way want to confirm different on mbstring PHP 8.1 or newer and PHP 8.0 or older, but when I port to PHP 8.0 from PHP 8.1 or newer phpt files, it stopped die() function when test failed. I want to make a list on .out file, so I don't want to stop it.

I way want to confirm different on mbstring PHP 8.1 or newer and
PHP 8.0 or older, but when I port to PHP 8.0 from PHP 8.1 or newer
phpt files, it stopped die() function when test failed. I want to
make a list, so I don't want to stop it.
@cmb69
Copy link
Member

cmb69 commented Nov 28, 2022

Hmm, that might make sense. @alexdowad, what do you think?

@alexdowad
Copy link
Contributor

Hmm, this is interesting.

I wonder if this should really be merged into master or if it can just be a local modification which @youkidearitai can use when investigating differences between PHP releases.

When developers are running the test suite, generally they want it to finish as quickly as possible. Immediately failing a test when one assertion is false helps the entire test suite to finish running sooner.

Also, when someone is working on a change to mbstring, I wonder if would really be helpful to get a huge .out file listing thousands of assertion failures, rather than just seeing the first assertion which was false. Maybe it would be useful in some cases?

I don't really have a strong opinion either way. If it is felt that tests should continue running and print all assertion failures before exiting, that shouldn't be a problem.

@youkidearitai
Copy link
Contributor Author

Thanks for explain about encoding_tests.inc. I explain about why I want to complete .out file.

PHP 8.1 (or newer) mbstring is many change to text encodings. I tried put it my website, then Japanese PHP users many reactions. If all (additional) phpt tests can't run, we won't see the changes and never know.

Therefore, I don't want stop to test when it failed.

@alexdowad
Copy link
Contributor

@youkidearitai, thanks for the comment.

If I understand well, what you really want is a way to find all behavior differences between two different releases of PHP. Is that correct?

Do you intend that going forward, you will be regularly running .phpt test files from one release of PHP under a different release of PHP to check if any differences are detected? Or is this something you only plan to do once?

@youkidearitai
Copy link
Contributor Author

If I understand well, what you really want is a way to find all behavior differences between two different releases of PHP. Is that correct?

Yes, correct.

Do you intend that going forward, you will be regularly running .phpt test files from one release of PHP under a different release of PHP to check if any differences are detected? Or is this something you only plan to do once?

We plan to do this when PHP 8.2 and 8.3 or later are released.

@alexdowad
Copy link
Contributor

We plan to do this when PHP 8.2 and 8.3 or later are released.

Well, then, no objection (I guess).

@youkidearitai If you are interesting in helping to ensure that there are no unintentional BC breaks in mbstring, and that any intentional changes can be easily noticed, probably the best thing you could do would be to contribute more unit tests for mbstring. Since I started maintaining the library, I have greatly increased the number of unit tests, but I think we still need a lot more.

@alexdowad
Copy link
Contributor

@youkidearitai, how about this. What if you add a new global variable which counts the number of failed assertions, and if it goes over some limit (maybe 1000) then call die.

Do you think that makes sense?

If you execute full test, set $testFailedLimit is -1 in
encoding_tests.inc.
@youkidearitai
Copy link
Contributor Author

youkidearitai commented Dec 19, 2022

@alexdowad Thank you very much for advice.

What if you add a new global variable which counts the number of failed assertions, and if it goes over some limit (maybe 1000) then call die.

I tried various tests, set limit is very useful and make sense. I try to implement that.
(My English is not well, welcome to comment for weird sentences.)

@alexdowad
Copy link
Contributor

@youkidearitai This looks good, thank you. I will merge it.

@alexdowad
Copy link
Contributor

@youkidearitai Would you be happy if this is merged into master, or do you want it to go into PHP-8.1, PHP-8.2, and master?

@youkidearitai
Copy link
Contributor Author

@alexdowad Thank you. Just the master branch is enough.

@alexdowad
Copy link
Contributor

Squashed into 1 commit and merged into master.

@alexdowad alexdowad closed this Dec 19, 2022
@youkidearitai youkidearitai deleted the yuyahamada_mbstring_dont_stop_test_faild branch December 19, 2022 14:46
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.

3 participants