This is a child issue of #3376057: [META] Add declare(strict_types=1) to all tests. After adding enabling strict types to all tests there were around 3000 errors. Fixing them all in one issue will lead to an enormous merge request that's difficult to review, as per the issue scope guidelines.
Create the rector.php file:
<?php
use Rector\Config\RectorConfig;
use Rector\TypeDeclaration\Rector\StmtsAwareInterface\DeclareStrictTypesRector;
return static function (RectorConfig $rectorConfig): void {
$rectorConfig->rule(DeclareStrictTypesRector::class);
};
Process all test traits:
composer require rector/rector --dev
php ./vendor/bin/rector process core/tests/Drupal/Tests/Traits
find core/tests/Drupal/Tests -type f -name "*Trait.php" -maxdepth 1 | xargs php ./vendor/bin/rector process
find core -type d -path "*/tests/src/Traits" | xargs php ./vendor/bin/rector process
Fix all strict type issues in test traits
Follow up [@todo create follow up issue]
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !5287
Comments
Comment #2
mstrelan commentedComment #4
mstrelan commentedComment #5
smustgrave commentedRan the script in the issue summary and got a different result
Comment #6
smustgrave commentedActually think I misunderstood the ticket. As far as your comments think refactoring any trait/test could be it's own ticket.
Comment #7
mstrelan commentedJust to clarify, pipeline 45766 demonstrates all the errors and fails when declare(strict_types=1) is added to all traits. The following commits then fix up the issues. Finally the initial commit is reverted so we have a reviewable MR. As discussed in #3397890-11: Fix strict type errors in unit tests we decided that splitting the fixes and adding the declaration should be separated. We need a follow up issue here for adding the declaration (see the todo in the IS), which can just be a cherry-pick of the first commit.
Comment #8
mstrelan commentedDid not mean to change the status
Comment #9
xjmUnder our test coding standards, it's preferred to use string literals when asserting test data. Neither
FormattableMarkup()norsprintf()should be used. (We've had massive cleanups previously to eliminate the usage of both in tests.) Thanks!Comment #10
mstrelan commented@xjm these are not in the actual assertions but in the $message argument.
Comment #11
xjm@mstrelan, it goes for all arguments of assertions including the message. Actually even more so for the message; we stripped string formatting off that eight years back, whereas we did so only 3-5 years ago for asserted values.
What would be best would be to use the literal values rather than unnecessary uses of randomly generated test data, but that's possibly out of scope. So, in the meanwhile, double-quoted strings. (Or this issue is small enough that I would also embrace going a step further and using literal fixture values for those few tests.)
Adding the related policy issue which apparently never got closed out and added to docs, but still applies.
Comment #12
mstrelan commentedNot trying to argue here as I'm not bothered either way. Wondering what you mean "we stripped string formatting off that eight years back" since this and all the other related issues are removing usage of FormattableMarkup.
Regardless, I've converted these to string interpolation and it's back to NR.
Comment #13
smustgrave commentedSeems replacement of sprintf has been done.
Comment #17
xjmLooks great now. So much cleaner. Committed to 11.x, 10.2.x, and 10.1.x as a test improvement. Thanks!