Changes were made through other issues; let's close and open a different one based on current state if needed.
mondrake → created an issue. See original summary → .
#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?
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.
mondrake → created an issue.
#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.
Regenerated patch and tagged 'Avoid commit conflicts' tag per https://drupal.slack.com/archives/C079NQPQUEN/p1752247969204049
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'.
mondrake → created an issue.
This touches 1,304 files, so it could become stale easily. I would encourage at reviewing as is also if not mergeable (at the end of the day this is again and again the same conversion pattern repeating), and then re-run the patch generation last minute before commit, if necessary. Otherwise we spend time running the same steps again with no value.
I suggest to wait for conversions of annotations to attributes to be completed, then check on attributes instead. Probably a PHPStan rule should be written here.
We need to convert all tests from annotation to attributes, first.
I'm afraid we have to reopen this is light of PHPUnit 12 - that is making final the constructor of TestCase. We can no longer override it to set programmatically at the base class level.
The issue was reported upstream but no alternatives were suggested: https://github.com/issues/created?issue=sebastianbergmann%7Cphpunit%7C5838
So I think we need to
- add the #[RunTestsInSeparateProcesses] attribute to all existing kernel and functional tests
- add a PHPStan rule that will prevent new kernel and functional tests to miss the attribute
Despite this reducing the number of CPUs, after 📌 Consider moving quickstart tests to build tests Active that moved the two tests to the Build suite, now 📌 Autodetect available CPUs in run-tests.sh for concurrency Active has a record execution time of 14 secs for core PHPUnit tests under PHP 8.5:
https://git.drupalcode.org/issue/drupal-3526459/-/jobs/5815987/viewer
8.3 and 8.4 are slightly below 30 secs, but I think it's the actual runner making the real difference here.
Sorry I can't understand #40 - tests are green and the comment seems not related to this issue. Can you elaborate please?
(not so) random recurring test failures
mondrake → created an issue.
Adjusted the docblocks.
good catch. on that
mondrake → created an issue.
I did not mean to open a fork… must have tapped something inadvertently.
mondrake → made their first commit to this issue’s fork.
mondrake → created an issue.
mondrake → created an issue.
mondrake → created an issue.
Legitimate test failure.
mondrake → created an issue.
IMHO it makes no sense to enforce test method description in docblocks via PHPCS, but rather than starting a code standards discussion I preferred to change the rector rule to create one (using PHPUnit's testdox NamePrettifier) when missing.
mondrake → created an issue.
Skipped in the patch generation script the files that lead to erroneous conversions, and applied the latest patch.
Self-RTBC as the patch was RTBC before and we just removed some hunks that will have to be converted manually.
Started a thread in Slack about how to scope further conversion issues https://drupal.slack.com/archives/C079NQPQUEN/p1751533999848609
Replied inline.
@xjm it would help if you could post how you would envisage a docblock to look exactly like in this case, WITHOUT the phpcs caveats. With that, we can try to decorate it with the phpcs:ignore directives that phpcs will tell us are necessary.
Thanks, this seems reasonable. Would you mind adding a note in the README to explain this?
This is not PHPStan level 0... it's PHPStan level 10. If you test locally the patch on the single core/lib/Drupal/Core/Database/Database.php file with PHPStan, and set the level to 10, it will pass.
I know it's a little bit futuristic but here I am exploring.
Wrt #4, on higher levels than what we have now PHPStan checks validity of the array keys being used in code against the array shape. Without @phpstan-type defined on the class level as an 'alias' of the array shape, we'd have to enter the full array shape every time it is used (16 times in this class). Readability would get definitely worse as well as the risk of errors (if you start using a new array key, you'd have to change all the instances).
There's interest in using @phpstan-type, see https://git.drupalcode.org/project/drupal/-/merge_requests/10809#note_51..., but probably not yet enough focus, see
https://www.drupal.org/project/drupal/issues/3497431#comment-16102077
📌
Deprecate TestDiscovery test file scanning, use PHPUnit API instead
Active
and
https://www.drupal.org/project/drupal/issues/3082239#comment-15153451
📌
Forbid limited length primary and unique keys, allow only in indexes
Needs work
(later comments)
BTW, such long array shapes probably hint at the opportunity to use value objects instead. I opened ✨ Introduce a ConnectionParameters object to store database connection parameters Active .
mondrake → created an issue.
Fixed, thank you
Ok for a follow up.
mondrake → created an issue.
Wonder if we can extend from the same class in the mysql module, with some adjustments. Most of the mysqli module is done with a similar pattern.
I left the .install file on purpose because I understood an hook was coming, and it is a near copy of the now defunct same file in mysql.
mondrake → created an issue.
mondrake → created an issue.
rebased