Run tests on PHP 8.3, fix dynamic property for PHP 8.2 and update test suite #40
Conversation
| <php> | ||
| <ini name="error_reporting" value="-1" /> | ||
| </php> |
There was a problem hiding this comment.
The tests for PHP 8.2 and PHP 8.3 are currently running infinitely and I narrowed it down to these three lines being the cause of this. For other pull requests these changes ran smoothly, any idea what is happening here @clue?
There was a problem hiding this comment.
This looks strange. Can you reproduce this locally? Can you try excluding some tests to see which test is causing this? Using groups or excludes or filters might be useful.
There was a problem hiding this comment.
Hello @clue, I tried debugging the code to find the cause of this issue and after seeing a new error thrown in line 76 in the CompositeConnection.php file I ended up looking in the changes made in PHP 8.2 and saw that the dynamic properties are deprecated, see https://www.php.net/releases/8.2/en.php#deprecate_dynamic_properties. I decided to add a new class variable private $remote after discussing the new situation with @SimonFrings. Tests are running green now. 👍
There was a problem hiding this comment.
I also updated my comment and explain the situation. Thank you @clue for giving me the hint to exclude groups. It was definitely faster for narrowing it down. 👍
SimonFrings
requested review from
clue and
SimonFrings
and removed request for
clue
yadaiio
changed the title
There was a problem hiding this comment.
The dynamic property seems to be an oversight from #37, thanks for fixing this 👍
| @@ -29,7 +29,7 @@ public function setUpConnector() | |||
| public function tearDownSSHClientProcess() | |||
| { | |||
| // run loop in order to shut down SSH client process again | |||
| \React\Async\await(\React\Promise\Timer\sleep(0.001)); | |||
| \React\Async\await(\React\Async\delay(0.001)); | |||
There was a problem hiding this comment.
Untested, invalid code. delay() returns a void and can't be passed to await(). Also, delay() requires correct Async package version.
Builds on top of #32, #37 and #39.
References: reactphp/socket#300, clue/reactphp-zenity#63, clue/reactphp-csv#33 and others.
The tests were failing in PHP 5.3 because of an unknown
React\Promise\Timer\sleep()function. In order to avoid this I updated this to use theReact\Async\delay()function instead.Additionally I saw the dynamic properties were deprecated since PHP 8.2, see https://www.php.net/releases/8.2/en.php#deprecate_dynamic_properties, so I had to add a new class variable in the
CompositeConnection.phpfile to avoid that the test for PHP 8.2 and PHP 8.3 run infinitely.