🇮🇹
Account created on 7 May 2011, over 14 years ago
#

Merge Requests

More

Recent comments

🇮🇹Italy mondrake 🇮🇹

This is a larger jump, including pphpstan-drupal now too. And, IMHO it's better keep PHPStan updated no-matter-what, because it prevents errors to be committed, not just shows the ones that exist.

🇮🇹Italy mondrake 🇮🇹

No manual changes, just rerun the script at 📌 [meta] Convert test metadata from annotation to attribites Active . Self-RTBCing and will ping @catch as per #21.

🇮🇹Italy mondrake 🇮🇹

Regenerated patch in https://git.drupalcode.org/issue/drupal-3446380/-/jobs/6294097, then reset this branch to 11.x with --hard option, applied the regenerated patch, and pushed to MR branch with--force option. 342 files affected.

🇮🇹Italy mondrake 🇮🇹

This is critical to support PHP 8.5.

🇮🇹Italy mondrake 🇮🇹

Added a job to test kernel/functional/build tests under PHP 8.5, allowing failures for now.

🇮🇹Italy mondrake 🇮🇹

Working on this to update to PHPUnit 11.5.34 that

Do not configure report_memleaks setting (which will be deprecated in PHP 8.5) for PHPT processes

🇮🇹Italy mondrake 🇮🇹

mondrake changed the visibility of the branch 3539366-fix-tx-isolation to hidden.

🇮🇹Italy mondrake 🇮🇹

PHPUnit 11.5.33 is out and contains the fix that will allow 📌 Deprecate expectDeprecation(), use PHPUnit's expectUserDeprecationMessage() instead Postponed to proceed. Let’s bump to that.

🇮🇹Italy mondrake 🇮🇹

https://github.com/sebastianbergmann/phpunit/issues/6102 is fixed upstream, so as soon as a release 11.5.33 hits the road this is actionable again

🇮🇹Italy mondrake 🇮🇹

@andypost now 📌 Bump PHPUnit to ^11.5.32 with all dependencies for testing with PHP 8.5 Active fixes some of the dependencies telated issues

🇮🇹Italy mondrake 🇮🇹

The remaining failures are due to issues in Prophecy and in PhpDocumentor.

🇮🇹Italy mondrake 🇮🇹

What more work is needed here?

🇮🇹Italy mondrake 🇮🇹

Nice! Could use a simple functional test to verify route returns expected http code and the returned json can be successfully decoded.

🇮🇹Italy mondrake 🇮🇹

I think this makes sense for now. It’s a small change, if it does not work it can be easily reverted.

In PHPUnit upstream, this is being proposed in #[Retry] attribute to support retrying flaky test #6182. That will allow working on a test class instead of the entire CI job, but surely will only happen in PHPUnit 12 or 13.

🇮🇹Italy mondrake 🇮🇹

mondrake made their first commit to this issue’s fork.

🇮🇹Italy mondrake 🇮🇹

I think we need to add an additional event for the autocommit on DDL, separate from the regular one, and still emit the regular one in the expected moment even if the autocommit already happened. This will make simpler moving post-transaction callbacks to event subscriptions.

AFK for a while, will come back later.

🇮🇹Italy mondrake 🇮🇹

I was unsure about BC when I added ::purge so that’s why it did not end up in the interface. If this is acceptable it’s certainly the right thing to do.

🇮🇹Italy mondrake 🇮🇹

Because I think we should check if the extending classes have the metadata implemented as the base class, and if not see why not and possibly convert to attributes.

🇮🇹Italy mondrake 🇮🇹

A check to prevent the use of annotation is already done by PHPUnit itself when it is configured to fail on PHPUnit deprecations. See 📌 Complete test annotations to attributes conversion for Drupal/Test/Component Active for an example.

IMHO we should not replicate such a check here.

🇮🇹Italy mondrake 🇮🇹

Well, the new rule introduces new errors, so either we fix them or we baseline them :)

🇮🇹Italy mondrake 🇮🇹

Because fixing the errors that this rule surfaces is not trivial. So better leave that to follow ups IMHO, while the rule prevents adding more leaks.

🇮🇹Italy mondrake 🇮🇹

@ceonizm confirmed that the MR here fixes 🐛 Error at the end of any PHPunit test Active

🇮🇹Italy mondrake 🇮🇹

#43 you mean manually? An idea could be to create via createTable a PostgreSQL table with a very long name. Then inspecting the database you would find a table with an abbreviated name 63 chars long. Any Database API select/insert/update/query etc with the long name in Drupal would return data from the real table with abbreviated name.

🇮🇹Italy mondrake 🇮🇹

Underlying code in HEAD changed significantly in the last year. Let's close this now.

🇮🇹Italy mondrake 🇮🇹

📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active has taken over here. We need an issue to remove PHPUnit 10 support, but let's create a new one instead of reverting this one to its early state.

🇮🇹Italy mondrake 🇮🇹

No longer necessary.

🇮🇹Italy mondrake 🇮🇹

Since run-tests.sh is no longer spawning subprocesses of run-tests.sh itself, we no longer need to persist the test result SQLite database on disk to allow parent-child processing. So, we can try using a :memory: SQLite database within the main run-tests.sh process.

🇮🇹Italy mondrake 🇮🇹

Discussed this issue status in Slack, https://drupal.slack.com/archives/C079NQPQUEN/p1752837466231169

Return this to Fixed as it was before reopening. PHPStan is already running on PHP 8.4 as part of separate issue, so I will file a separate issue for the follow-up currently open.

🇮🇹Italy mondrake 🇮🇹

To be precise, PHP 8.4 is not a strict requirement. PHPUnit 11 works with PHP 8.3. as well (core tests are by default executed on 8.3). This issue 📌 Switch the default test environment to PHP 8.4 Active is going to change the default to PHP 8.4.

🇮🇹Italy mondrake 🇮🇹

@fjgarlin I commented inline - can you please elaborate the suggestion?

🇮🇹Italy mondrake 🇮🇹

Think this can be closed now.

🇮🇹Italy mondrake 🇮🇹

Replied inline. Yes, it's odd. Hopefully one day we will get rid of the test results database and of the --sqlite argument in the run-tests.sh script.

🇮🇹Italy mondrake 🇮🇹

Changes were made through other issues; let's close and open a different one based on current state if needed.

🇮🇹Italy mondrake 🇮🇹

#17

1. No. We need a meta and reparenting all these issues to that. I will open one.
2. Not that I am aware of.
3. Not that I am aware of.
4. No plan. I can only comment that PHPUnit 10+ does have a #[CoversMethod], but that only can be set at the class level. So theoretically tests classes could be split per each method that wants to be covered in detail, but that is a lot of work, and manual one. Maybe a policy issue for discusssion?

🇮🇹Italy mondrake 🇮🇹

Changed mind and added a PHPStan rule checking for either annotations or attributes on abstract test base classes. We have 17. errors to be solved.

🇮🇹Italy mondrake 🇮🇹

#14 thanks, I think we need to accept that commits still add annotations for now, it's too big to stop. We'll enforce later by enabling PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION: true like we're doing in 📌 Complete test annotations to attributes conversion for Drupal/Test/Component Active for component tests. But too early now.

🇮🇹Italy mondrake 🇮🇹

PHPStan fails in latest pipeline because, since this issue was RTBCed, issue 🐛 The migrate Row class API is incomplete Active has introduced a test method with 2 @covers annotations. Given this MR removed the @coversDefaultClass annotation from the class, the annotations added do not have a reference class anymore and therefore PHPStan cannot identify to what methods they refer to.

This MR touches 342 files, can therefore become easily stale, and it's requiring non-trivial work to reroll: it's not a rebase, but a full regeneration of the automated patch, reset of the branch, application of the patch, and force push.

I'll ask in Slack if it's worth tagging the issue 'avoid commit conflicts'.

Production build 0.71.5 2024