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

Run Infection on PHP 8.1. #1535

Merged
merged 16 commits into from Oct 13, 2021
Merged

Run Infection on PHP 8.1. #1535

merged 16 commits into from Oct 13, 2021

Conversation

sidz
Copy link
Member

@sidz sidz commented Jul 12, 2021

This PR:

  • Runs Infection on PHP 8.1.

@sidz sidz marked this pull request as draft July 12, 2021 20:52
@sidz
Copy link
Member Author

sidz commented Jul 12, 2021

latest release nikic/PHP-Parser still has an issue with parsing parameter passed by reference. https://github.com/infection/infection/pull/1535/checks?check_run_id=3050732900#step:7:79

Already fixed in master: nikic/PHP-Parser@c758510

waiting for release.

@maks-rafalko
Copy link
Member

waiting for release.

https://github.com/nikic/PHP-Parser/releases/tag/v4.12.0

@@ -16,7 +16,7 @@ jobs:

strategy:
matrix:
php-version: ['7.4']
Copy link
Member

Choose a reason for hiding this comment

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

7.4 is still supported until 28 of November, 2021, and it's used by ~60% of Infection users, I think we should not get rid of it now.

Or, alternatively, if we want to get rid (personally, I'm ok with that) - we should bump the version in composer.json to ^8.0, here

https://github.com/infection/infection/blob/master/composer.json#L45

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted

- name: Cache dependencies
uses: actions/cache@v2
with:
path: ${{ steps.composer-cache.outputs.dir }}
path: ~/.cache/composer
Copy link
Member

Choose a reason for hiding this comment

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

This won't work on Windows IIRC, and we're testing on Windows.

@sidz sidz force-pushed the run-tests-on-php-8.1 branch 2 times, most recently from e240bca to ae5911b Compare October 7, 2021 18:47
@sidz sidz force-pushed the run-tests-on-php-8.1 branch 7 times, most recently from b9d2d3c to bdc579b Compare October 11, 2021 20:34
@@ -18,16 +18,15 @@ jobs:
matrix:
operating-system: [ubuntu-latest]
php-version: ['7.4']
dependencies: ['']
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer required as we got rid of --ignore-platform-req=php

use Symfony\Component\Finder\SplFileInfo;
use ReturnTypeWillChange;

final class MockSplFileInfo extends SplFileInfo
Copy link
Member Author

Choose a reason for hiding this comment

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

copy idea from symfony repo.

@@ -43,7 +44,7 @@ final class IterableCounterTest extends TestCase
{
public function test_it_does_not_count_when_not_asked(): void
{
$iterator = $this->createMock(Iterator::class);
$iterator = new ArrayIterator();
Copy link
Member Author

Choose a reason for hiding this comment

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

use real iterator to avoid deprecation notes in PHP 8.1

@sidz sidz self-assigned this Oct 11, 2021
@sidz sidz marked this pull request as ready for review October 11, 2021 21:16
@sidz
Copy link
Member Author

sidz commented Oct 11, 2021

finally ready for review 🤓

Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

mostly questions, besides that - great work 👍

@@ -67,8 +67,7 @@
"webmozart/path-util": "^2.3"
},
"conflict": {
"phpunit/php-code-coverage": ">9 <9.1.4",
"symfony/console": "=4.1.5"
Copy link
Member

Choose a reason for hiding this comment

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

could you please clarify why was it removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -58,6 +58,10 @@ abstract class AbstractUnwrapMutator implements Mutator
final public function mutate(Node $node): iterable
{
foreach ($this->getParameterIndexes($node) as $index) {
if ($node->args[$index] instanceof Node\VariadicPlaceholder) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I got this change. Do we need a test (at least for 1 such case) that will proof it's needed?

Copy link
Member Author

@sidz sidz Oct 12, 2021

Choose a reason for hiding this comment

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

due to changes in PHP-Parser 4.13. Look at 3rd option in the Added group (https://github.com/nikic/PHP-Parser/releases):

p.s. looks like we can use getArgs() instead of this trick. I'll give it a try

Copy link
Member Author

Choose a reason for hiding this comment

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

such conditions is required to avoid issues like https://github.com/infection/infection/pull/1535/checks?check_run_id=3867799614#step:7:10

also can't use getArgs due to https://github.com/infection/infection/pull/1535/checks?check_run_id=3867799614#step:7:32

p.s. so reverted to previous commit.

*/
private function createSplFileInfosTraversable(array $filePaths): Traversable
{
return take($filePaths)
->map(function (string $filename) {
$traceMock = $this->createMock(SplFileInfo::class);
Copy link
Member

Choose a reason for hiding this comment

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

is it not working anymore? what's the reason of adding MockSpliFileInfo?

Copy link
Member Author

@sidz sidz Oct 12, 2021

Choose a reason for hiding this comment

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

to avoid tons of deprecation notes in PHP 8.1. like: Return type of Mock_SplFileInfo_8d3a072c::getPath() should either be compatible with SplFileInfo::getPath(): string, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

unfortunately I can't give you a link to failed tests in our pipeline as they are not available.

@maks-rafalko maks-rafalko enabled auto-merge (squash) October 13, 2021 06:53
@maks-rafalko
Copy link
Member

@sidz could you please rebase? I think we should merge it to avoid conflicts and failed builds on master (enabled auto-merge)

@sidz
Copy link
Member Author

sidz commented Oct 13, 2021

@maks-rafalko done

@sidz
Copy link
Member Author

sidz commented Oct 13, 2021

looks like Github has some issues with builds based on windows. Same on master

@maks-rafalko maks-rafalko merged commit 55446cf into master Oct 13, 2021
@maks-rafalko maks-rafalko deleted the run-tests-on-php-8.1 branch October 13, 2021 11:19
@maks-rafalko
Copy link
Member

I've re-run builds and now they are green, seems like just an occasional issue on GHA

Thank you, @sidz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants