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

Updating to phpunit 10 #7339

Open
wants to merge 4 commits into
base: 2.6
Choose a base branch
from
Open

Updating to phpunit 10 #7339

wants to merge 4 commits into from

Conversation

mamazu
Copy link
Contributor

@mamazu mamazu commented Mar 31, 2024

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets closes #6988
Related issues/PRs -
License MIT
Documentation PR -

What's in this PR?

Updating phpunit to version 10 and fixing the tests

  • Updating the dependency in the composer.json
  • Running rector to upgrade the tests (it's using attributes instead of annotations now)
  • Fixing the problems with the dataprovider functions being static now (mostly)

Why?

It's always a good idea to have an up to date testing library.

To Do

  • Fix the tests

@mamazu mamazu force-pushed the phpunit-upgrade branch 2 times, most recently from 8c367cb to 3c127eb Compare March 31, 2024 14:14
@mamazu mamazu changed the title updating to phpunit 10 (mostly) Updating to phpunit 10 Mar 31, 2024
@mamazu mamazu force-pushed the phpunit-upgrade branch 4 times, most recently from d433aa5 to b612f25 Compare March 31, 2024 15:24
Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

I did not yet review the whole part. I think we should cherry pick only the required changes out of this pull request. We should not do any type refactoring in it as that makes review here hard and too big, we should keep such things seperated.

On some places prophesize was replaced with PHPUnit mock, we are still using prophosize so that replacement is not required and should stay as it is at current state.

src/Sulu/Bundle/WebsiteBundle/phpunit.xml.dist Outdated Show resolved Hide resolved
src/Sulu/Bundle/WebsiteBundle/phpunit.xml.dist Outdated Show resolved Hide resolved
@@ -179,7 +177,7 @@ private function createTag($name)
return $tag;
}

private function createMedia($title, $collection, $mimeType, $type, $tags = [], $targetGroups = [])
private function createMedia($title, $collection, $mimeType, $type, $tags = [], $targetGroups = []): Media
Copy link
Member

Choose a reason for hiding this comment

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

we should reallly focus just on the PHPUnit update this get lot of hard to review things if we do here such refactorings.

Copy link
Member

Choose a reason for hiding this comment

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

test method changes should be done on 2.5 branch if possible and non breaking changes

@alexander-schranz alexander-schranz added the DX Affecting the end developer label Apr 2, 2024
@mamazu mamazu mentioned this pull request Apr 3, 2024
@mamazu
Copy link
Contributor Author

mamazu commented Apr 7, 2024

Todos:

  • You wanted to discuss internally what the best way to go about mocking is
  • Check if the failing tests should be failing
  • PhpUnit Config file needs to be reformatted

@mamazu mamazu force-pushed the phpunit-upgrade branch 2 times, most recently from 456b658 to e5365b0 Compare April 9, 2024 07:12
@mamazu mamazu force-pushed the phpunit-upgrade branch 5 times, most recently from f11f153 to b604859 Compare May 10, 2024 16:55
@mamazu mamazu force-pushed the phpunit-upgrade branch 6 times, most recently from 3939947 to 98a2ab4 Compare May 15, 2024 10:01
phpunit.xml.dist Outdated
bootstrap="./vendor/autoload.php"
colors="true"
failOnIncomplete="true"
failOnWarning="true"
failOnRisky="true"
>
<coverage>
cacheDirectory=".phpunit.cache">
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to configure this? Would keep this on default if possible.

To make switching between 2.5 <-> 2.6 easier for this who don't have such files in there global gitignore we should add the directory also to the .gitignore file of the 2.5 branch. Can you target a merge request for it?

Copy link
Contributor Author

@mamazu mamazu May 15, 2024

Choose a reason for hiding this comment

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

I mean it's the default value for phpunit so we don't have to specify it.

Copy link
Member

Choose a reason for hiding this comment

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

Then I would remove the config and just add the new directory to the .gitignore file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just saw that the cache file is already in the .gitignore

phpunit.xml.dist Outdated Show resolved Hide resolved
Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

Some changes I would backport to the 2.5 branch. Mostly this pull request for 2.6 should then contain only phpunit.xml required changes and data provider changes.

Comment on lines +58 to +60
yield ['sulu_io', 'test-1', ['property1' => 'test1', 'property2' => 'test2']];
yield ['sulu_io', 'test-2', null];
yield ['sulu_io', 'test-3', self::createStub(NodeInterface::class)];
Copy link
Member

Choose a reason for hiding this comment

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

Can we do such changes on the 2.5 branch?

$dir = __DIR__ . '/../../../../../../tests/Resources';

return $dir;
return __DIR__ . '/../../../../../../tests/Resources';
Copy link
Member

Choose a reason for hiding this comment

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

Can we do such changes on the 2.5 branch?

Comment on lines +119 to +120
yield ['sulu_io', 'test-1', '123-123-123', true];
yield ['sulu_io', 'test-1', '123-123-123', false];
Copy link
Member

Choose a reason for hiding this comment

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

Can we do such changes on the 2.5 branch?

Comment on lines +89 to +91
$webspaceKey = 'sulu_io';
$key = 'test-1';

Copy link
Member

Choose a reason for hiding this comment

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

??? looks like unexpected change

*/
public function testSave($webspaceKey, $key, $data): void
#[\PHPUnit\Framework\Attributes\DataProvider('dataProvider')]
public function testSave(string $webspaceKey, string $key, array|NodeInterface|null $data): void
Copy link
Member

Choose a reason for hiding this comment

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

We should do such changes on the 2.5 branch

@@ -179,7 +177,7 @@ private function createTag($name)
return $tag;
}

private function createMedia($title, $collection, $mimeType, $type, $tags = [], $targetGroups = [])
private function createMedia($title, $collection, $mimeType, $type, $tags = [], $targetGroups = []): Media
Copy link
Member

Choose a reason for hiding this comment

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

test method changes should be done on 2.5 branch if possible and non breaking changes

Comment on lines +68 to +73
array $targetGroups,
array $ruleWhitelists,
?string $webspaceKey,
?TargetGroup $evaluatedTargetGroup,
int $frequency = TargetGroupRuleInterface::FREQUENCY_SESSION,
?TargetGroup $currentTargetGroup = null
Copy link
Member

Choose a reason for hiding this comment

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

test method changes should be done on 2.5 branch if possible and non breaking changes

@@ -99,7 +97,7 @@ public function testEvaluate(
$this->assertEquals($evaluatedTargetGroup, $this->targetGroupEvaluator->evaluate($frequency, $currentTargetGroup));
}

public static function provideEvaluationData()
public static function provideEvaluationData(): array
Copy link
Member

Choose a reason for hiding this comment

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

test method changes should be done on 2.5 branch if possible and non breaking changes

@@ -441,7 +429,7 @@ public function testAddTargetGroupHitScript(
$this->assertEquals('<body><script></script></body>', $response->getContent());
}

public static function provideAddTargetGroupHitScript()
public static function provideAddTargetGroupHitScript(): array
Copy link
Member

Choose a reason for hiding this comment

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

test method changes should be done on 2.5 branch if possible and non breaking changes

Comment on lines -33 to -36

<listeners>
<listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener"/>
</listeners>
Copy link
Member

Choose a reason for hiding this comment

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

I would also remove this listener on the 2.5 to be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that in #7432

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

Successfully merging this pull request may close these issues.

PHPUnit 10 Upgrade
2 participants