PhpUnit 10 Compatibility Part 1 #3523
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is not a change to move to PhpUnit 10. There is no compelling reason to do so at this time, although it is bound to happen eventually. There are a staggering number of problems (somewhere around 3,000) with the current test suite under PhpUnit 10; this is an attempt to get ahead of the curve by addressing them now.
Method `setOutputCallback` has gone away. This affects only Helper/SampleTest. It appears that `ob_start` and its allies provide an effective equivalent. FWIW, the absence of `setOutputCallback` is a good indication of whether or not PhpUnit 10 is in use, and I will use that fact in a few tests.
Class `ComplexAssert` with no constructor, and always used with `new ComplexAssert()`, extends `TestCase`. Apparently, the constructor for TestCase requires an argument, and PhpUnit 10 complains about not supplying one. Adding an empty constructor to ComplexAssert avoids this problem.
There are two very minor source changes, to Calculation/Calculation and Reader/Xlsx, where problems were exposed with PhpUnit 10 that had not been previously been exposed. AFAIK, these are the only source changes required; the rest of the changes are to test members.
The bulk of the problems are because PhpUnit 10 insists that provider methods be static. Most of those can be changed by a script without any further action; those changes will constitute the Part 2 counterpart of this PR. In this PR you will find the exceptional cases that can't be automated for one reason or another. The tests for Database functions have mild complications that are easily handled. Most of the other provider changes in this PR are because the method names didn't follow an established pattern ('provider' isn't part of the method name); those are also easily handled manually. Modifying the following tests provided significant challenges:
- Writer/Xls/WorkbookTest testAddColor
- Worksheet/Table/TableTest testSetRangeValidRange
The handling of warning messages issued by the code differs in PhpUnit 10. According to the change log, "This means that using PHP functionality which triggers E_DEPRECATED, E_NOTICE, E_STRICT, or E_WARNING or calling code which triggers E_USER_DEPRECATED, E_USER_NOTICE, or E_USER_WARNING can no longer hide a bug in your code." To me, the effect of that change seems to be exactly the opposite - such messages were available to the test with PhpUnit 9 (so we could test for them), and are no longer available (so we can't). I haven't even succeeded with a custom error message handler as part of the script. I will continue to investigate, but, for now, will skip some tests under PhpUnit 10 for the following:
- Shared/OleTest testChainedWriteMode and testChainedBadPath
- Reader/Html/HtmlLoadStringTest testLoadInvalidString
- Reader/Html/HtmlTest testBadHtml
MarkBaker
reviewed
Apr 13, 2023
Parent construct suggested by @MarkBaker.
|
As usual, no concerns about Scrutinizer "complexity" complaint. |
Warning (and other) messages are handled differently in PhpUnit 10 than in earlier versions.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Apr 19, 2023
Successor to PR PHPOffice#3523. There are 494 single-line changes (`public function provider` to `public static function provider`) in this PR. None of these were made manually; they were all created with the following script (adapted from https://stackoverflow.com/questions/25909820/how-to-recursively-iterate-through-files-in-php): ```php $dir = 'C:/git/unit10prep2/tests/PhpSpreadsheetTests'; $it = new RecursiveDirectoryIterator($dir); // Loop through files foreach(new RecursiveIteratorIterator($it) as $file) { if ($file->getExtension() === 'php') { $contents = file_get_contents($file); $new = preg_replace('/public function (\\w*)([Pp])rovider/', 'public static function $1$2rovider', $contents); if ($new !== $contents) { echo "changing $file\n"; file_put_contents($file, $new); } } } ``` After this PR, there will be one more, with a small number of test changes, and enabling PhpUnit 10 for Php 8.1+.
12 tasks
oleibman
added a commit
that referenced
this pull request
Apr 20, 2023
Successor to PR #3523. There are 494 single-line changes (`public function provider` to `public static function provider`) in this PR. None of these were made manually; they were all created with the following script (adapted from https://stackoverflow.com/questions/25909820/how-to-recursively-iterate-through-files-in-php): ```php $dir = 'C:/git/unit10prep2/tests/PhpSpreadsheetTests'; $it = new RecursiveDirectoryIterator($dir); // Loop through files foreach(new RecursiveIteratorIterator($it) as $file) { if ($file->getExtension() === 'php') { $contents = file_get_contents($file); $new = preg_replace('/public function (\\w*)([Pp])rovider/', 'public static function $1$2rovider', $contents); if ($new !== $contents) { echo "changing $file\n"; file_put_contents($file, $new); } } } ``` After this PR, there will be one more, with a small number of test changes, and enabling PhpUnit 10 for Php 8.1+.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Apr 21, 2023
Final changes for PhpUnit 10, including enabling it for testing. This finishes the work of PR PHPOffice#3523 and PR PHPOffice#3526. The major remaining problem with PhpUnit 10 is that earlier releases converted notices and warnings to exceptions, and 10 does not. Not having the information provided by the messages seems risky to me. Fortunately, it appears that you can add an error handler to the test bootstrap for 10 and make it act like earlier versions; I have done so. In order to demonstrate the effectiveness of this handler, a new otherwise unused class Helper/Handler and tests for that class are added. As part of the testing of this change, it became apparent that the fopen in OLE::getStream attempts to create a dynamic property $context in Shared/OLE/ChainedBlockStream, and that action is deprecated in Php8.2. Adding the property to the class eliminates that problem. No executable code is added, and this is the only change to source code. There also seems to have been a change in assertXmlStringEqualsXmlString in PhpUnit 10. The only test which uses that method is Chart/Issue589Test, and both the places which use that method could just as easily and effectively use assertSame. They are changed to do so.
12 tasks
oleibman
added a commit
that referenced
this pull request
Apr 28, 2023
* PhpUnit 10 Compatibility Part 3 (Last) Final changes for PhpUnit 10, including enabling it for testing. This finishes the work of PR #3523 and PR #3526. The major remaining problem with PhpUnit 10 is that earlier releases converted notices and warnings to exceptions, and 10 does not. Not having the information provided by the messages seems risky to me. Fortunately, it appears that you can add an error handler to the test bootstrap for 10 and make it act like earlier versions; I have done so. In order to demonstrate the effectiveness of this handler, a new otherwise unused class Helper/Handler and tests for that class are added. As part of the testing of this change, it became apparent that the fopen in OLE::getStream attempts to create a dynamic property $context in Shared/OLE/ChainedBlockStream, and that action is deprecated in Php8.2. Adding the property to the class eliminates that problem. No executable code is added, and this is the only change to source code. There also seems to have been a change in assertXmlStringEqualsXmlString in PhpUnit 10. The only test which uses that method is Chart/Issue589Test, and both the places which use that method could just as easily and effectively use assertSame. They are changed to do so. * Remove Phpunit Verbose Option Not supported in PhpUnit 10. I'm not at all sure that this is the correct solution for this problem. * Try Changing Phpunit Command for Different Php Releases Not sure how to test this locally. We'll see if it works on github. * Another main.yml attempt We shall see. * Eliminate One Test Not sure why testDeprecated is not working in Github; it works locally. Disable it for now and continue to research. * Show Incomplete and Other Messages in PhpUnit 10 6 new command line args to replace the 1 they got rid of. * Restore Disabled Test Deprecated messages are suppressed by default setting for error_reporting. Switch that to E_ALL in bootstrap and restore original test. * Add Deprecation Tests for PhpUnit 9- Default configuration option caused deprecation messages to be suppressed. Change the option.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not a change to move to PhpUnit 10. There is no compelling reason to do so at this time, although it is bound to happen eventually. There are a staggering number of problems (somewhere around 3,000) with the current test suite under PhpUnit 10; this is an attempt to get ahead of the curve by addressing them now.
Method
setOutputCallbackhas gone away. This affects only Helper/SampleTest. It appears thatob_startand its allies provide an effective equivalent. FWIW, the absence ofsetOutputCallbackis a good indication of whether or not PhpUnit 10 is in use, and I will use that fact in a few tests.Class
ComplexAssertwith no constructor, and always used withnew ComplexAssert(), extendsTestCase. Apparently, the constructor for TestCase requires an argument, and PhpUnit 10 complains about not supplying one. Adding an empty constructor to ComplexAssert avoids this problem.There are two very minor source changes, to Calculation/Calculation and Reader/Xlsx, where problems were exposed with PhpUnit 10 that had not been previously been exposed. AFAIK, these are the only source changes required; the rest of the changes are to test members.
The bulk of the problems are because PhpUnit 10 insists that provider methods be static. Most of those can be changed by a script without any further action; those changes will constitute the Part 2 counterpart of this PR. In this PR you will find the exceptional cases that can't be automated for one reason or another. The tests for Database functions have mild complications that are easily handled. Most of the other provider changes in this PR are because the method names didn't follow an established pattern ('provider' isn't part of the method name); those are also easily handled manually. Modifying the following tests provided significant challenges:
The handling of warning messages issued by the code differs in PhpUnit 10. According to the change log, "This means that using PHP functionality which triggers E_DEPRECATED, E_NOTICE, E_STRICT, or E_WARNING or calling code which triggers E_USER_DEPRECATED, E_USER_NOTICE, or E_USER_WARNING can no longer hide a bug in your code." To me, the effect of that change seems to be exactly the opposite - such messages were available to the test with PhpUnit 9 (so we could test for them), and are no longer available (so we can't). The following tests now exist in both a version intended for PhpUnit9 or less (these could be dropped but I don't like to do that without a very strong reason) and a version intended for all releases:
This is:
Checklist:
Why this change is needed?
Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.