Skip to content

run tests using PHPUnit 11.5 #58370

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

Open
wants to merge 3 commits into
base: 7.3
Choose a base branch
from

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Sep 24, 2024

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

@xabbuh
Copy link
Member Author

xabbuh commented Sep 24, 2024

The PR cannot be merged at the moment as it is based on a bunch of other open pull requests that are paving the way to be able to actually run the tests with PHPUnit 11.

@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 2 times, most recently from 7178ae5 to 76245c5 Compare September 24, 2024 07:31
@alexandre-daubois
Copy link
Member

This is super nice news! Did you already take care of everything required to be "PHPUnit 11 compliant" in already opened PRs ?

@xabbuh
Copy link
Member Author

xabbuh commented Sep 24, 2024

@alexandre-daubois The "ensure session storages are opened before destroying them" commit is the only one that is not already part of an open PR. I do not really understand yet why that change is necessary (the flow in AbstractSessionHandler is different as headers_sent() returns a different result).

@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch from 76245c5 to 24d86e8 Compare September 24, 2024 11:18
@xabbuh xabbuh requested a review from chalasr as a code owner September 24, 2024 11:18
nicolas-grekas added a commit that referenced this pull request Sep 25, 2024
This PR was merged into the 5.4 branch.

Discussion
----------

Tweak error/exception handler registration

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #53812
| License       | MIT

This should allow removing the custom bootstrap file from #58370 /cc `@xabbuh`

The change on FrameworkBundle leads to a tweaked behavior: we don't override the previous error handler in case it's not the Symfony one. This shouldn't change anything in practice since the error handler is already registered by the runtime component.

The rest is closer to bug fixes.

Commits
-------

af9c035 Tweak error/exception handler registration
@nicolas-grekas
Copy link
Member

We need a way to register DebugClassLoader, and maybe a few other things. Basically what we do in SymfonyTestsListenerTrait.

@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 2 times, most recently from ab2f604 to e56f932 Compare September 25, 2024 16:42
@xabbuh xabbuh changed the title run integration tests using PHPUnit 11 [WIP] run integration tests using PHPUnit 11 Sep 27, 2024
@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch from e56f932 to bddc3f5 Compare September 27, 2024 11:30
@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 5 times, most recently from d72eadc to c89477a Compare October 10, 2024 06:49
@xabbuh xabbuh changed the title [WIP] run integration tests using PHPUnit 11 run integration tests using PHPUnit 11 Oct 10, 2024
@xabbuh xabbuh changed the title run integration tests using PHPUnit 11 [WIP] run integration tests using PHPUnit 11 Oct 10, 2024
@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 9 times, most recently from 56d703c to eb73303 Compare April 20, 2025 21:42
@xabbuh xabbuh changed the title [WIP] run tests using PHPUnit 11.5 run tests using PHPUnit 11.5 Apr 21, 2025
@xabbuh
Copy link
Member Author

xabbuh commented Apr 21, 2025

@symfony/mergers The PR is finally ready to be reviewed.

Status: Needs Review


env:
COMPOSER_NO_INTERACTION: '1'
SYMFONY_DEPRECATIONS_HELPER: 'strict'
Copy link
Member

Choose a reason for hiding this comment

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

this env var should not be necessary anymore, as it won't be used with PHPUnit 11

Copy link
Member Author

Choose a reason for hiding this comment

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

removed it

@@ -76,7 +76,7 @@ jobs:
([ -d "$COMPOSER_HOME" ] || mkdir "$COMPOSER_HOME") && cp .github/composer-config.json "$COMPOSER_HOME/config.json"

echo COLUMNS=120 >> $GITHUB_ENV
echo PHPUNIT="$(pwd)/phpunit --exclude-group tty,benchmark,intl-data,integration" >> $GITHUB_ENV
echo PHPUNIT="$(pwd)/phpunit --exclude-group tty --exclude-group benchmark --exclude-group intl-data --exclude-group integration --fail-on-deprecation" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

I suggest configuring --fail-on-deprecation through the config file instead, so that it is also failing when running locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -140,7 +140,7 @@ jobs:
[[ "${{ matrix.mode }}" = *-deps ]] && mv composer.json.phpunit composer.json || true

if [[ "${{ matrix.mode }}" = low-deps ]]; then
echo SYMFONY_PHPUNIT_REQUIRE="nikic/php-parser:^4.18" >> $GITHUB_ENV
echo SYMFONY_PHPUNIT_REQUIRE="nikic/php-parser:^5.0" >> $GITHUB_ENV
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 still have a job testing our compat with nikic/php-parser v4 for our code using it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am afraid we cannot: #58370 (comment):

PHPUnit 11 (through its dependency on phpunit/php-code-coverage is not compatible with nikic/php-parser 4

Copy link
Member

Choose a reason for hiding this comment

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

Only Translation supports v4, I would bump it to ^5 on 7.3

Copy link
Member Author

Choose a reason for hiding this comment

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

see #60255

@@ -251,9 +250,7 @@ public static function idsProvider(): iterable
yield ['foo'];
}

/**
* @group legacy
Copy link
Member

Choose a reason for hiding this comment

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

should we keep a legacy group for our tests covering deprecated behaviors ? Our deps=high job is excluding this group when running tests for a version that is not part of the last major version (as the dependencies could be installed at the next major version already):

# Legacy tests are skipped when deps=high and when the current branch version has not the same major version number as the next one
[[ "${{ matrix.mode }}" = high-deps && $SYMFONY_VERSION = *.4 ]] && echo LEGACY=,legacy >> $GITHUB_ENV || true

Copy link
Member

Choose a reason for hiding this comment

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

thinking about this again, I think this option was overkill and was leading to not testing some parts of the code in CI in our LTS branches. Many legacy tests we have are about testing deprecations of their own component, which does not need such exclusion. Tests relying on cross-package deprecations could be responsible for skipping themselves when needed.

/**
* @group legacy
*/
#[IgnoreDeprecations]
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR, but we should update those tests. I'm quite sure most of them are not legacy ones that are supposed to be removed in 8.0 (but they have not been updated to use named arguments for the constraint constructor)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm quite sure that they can indeed be removed. Before #54744 we had data providers configuring the constraints with both ways (named arguments and associative options array) to configure them, but I split those tests into two when deprecating the non named arguments way.

@@ -18,7 +17,7 @@
</testsuite>
</testsuites>

<coverage>
<source>
Copy link
Member

Choose a reason for hiding this comment

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

We need the ignoreSuppressionOfDeprecations="true" parameter in the component-level config files as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -17,6 +17,9 @@
use Symfony\Bridge\PhpUnit\DeprecationErrorHandler\DeprecationGroup;
use Symfony\Component\ErrorHandler\DebugClassLoader;

/**
* @requires PHPUnit < 10
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this have the attribute as well, so that PHPUnit 11 excludes it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

in fact, I even think that having the attribute without the annotation should be sufficient

@@ -67,9 +68,8 @@ public function testInvalidValues($value, $message, $line)

/**
* When deprecations are skipped by the validator, the testsuite reporter will catch them so we need to mark the test as legacy.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* When deprecations are skipped by the validator, the testsuite reporter will catch them so we need to mark the test as legacy.
* When deprecations are skipped by the validator, the testsuite reporter will catch them so we need to mark the test as ignoring deprecations.

# HttpClient tests need to run separately, they block when run with other components' tests concurrently
php phpunit src\Symfony\Component\HttpClient || ($x = 1)

exit $x

windows-all-extensions:
Copy link
Member

Choose a reason for hiding this comment

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

To make things easier to maintain, would it be worth backporting this split of the Windows jobs ? what do you think @nicolas-grekas ?

Copy link
Member Author

@xabbuh xabbuh Apr 22, 2025

Choose a reason for hiding this comment

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

we talked about that in #58721 but decided to not split the two test runs as we would otherwise run many of the tests twice since the phpunit.skipped would not be shared

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any discussion about that in the PR you linked

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, maybe @nicolas-grekas and me talked about that on Slack directly 🤔

@stof
Copy link
Member

stof commented Apr 22, 2025

Tests are not actually running with PHPUnit 11.5: https://github.com/symfony/symfony/actions/runs/14563576793/job/40849847611?pr=58370#step:6:44

@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 4 times, most recently from 70f5b92 to 3e19ea6 Compare April 22, 2025 08:46
@xabbuh
Copy link
Member Author

xabbuh commented Apr 22, 2025

Tests are not actually running with PHPUnit 11.5: https://github.com/symfony/symfony/actions/runs/14563576793/job/40849847611?pr=58370#step:6:44

Isn't that related to how our CI is configured? I mean the previous commit that is checked out does not have the changes needed in phpunit to install PHPUnit 11.5.

@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 2 times, most recently from 39cc605 to ca642e4 Compare April 23, 2025 05:17
@@ -30,7 +30,7 @@
"symfony/dependency-injection": "^6.4|^7.0",
"symfony/doctrine-messenger": "^6.4|^7.0",
"symfony/expression-language": "^6.4|^7.0",
"symfony/form": "^6.4.6|^7.0.6",
"symfony/form": "^7.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

because we need #58357

fabpot added a commit that referenced this pull request Apr 23, 2025
This PR was merged into the 7.3 branch.

Discussion
----------

[Translation] drop support for nikic/php-parser 4

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

Extracted from #58370, background: PHPUnit 11 is no longer compatible with `nikic/php-parser` 4 (see #58370 (comment)).

Commits
-------

2676ce9 drop support for nikic/php-parser 4
@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch 2 times, most recently from 03885db to 890b854 Compare April 24, 2025 18:13
@xabbuh xabbuh force-pushed the phpunit11-integration-tests branch from 890b854 to 8f0b304 Compare April 24, 2025 19:19
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.

8 participants