[meta] Fix strict type errors in kernel tests [#3397905]

Problem/Motivation

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.

Steps to reproduce

Add to phpcs.xml.dist:

  <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes">
    <properties>
      <property name="spacesCountAroundEqualsSign" value="0" />
    </properties>
    <include-pattern>*/tests/src/Kernel/*</include-pattern>
    <include-pattern>./tests/Drupal/KernelTests/*</include-pattern>
  </rule>

Run phpcbf:

composer phpcbf

Run the test suite:

./vendor/bin/phpunit -c core/phpunit.xml.dist --bootstrap=core/tests/bootstrap.php --testsuite=kernel

Proposed resolution

Fix all strict type issues in kernel tests
Follow up #3399388: Add declare(strict_types=1) to all Kernel tests

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#12	3397905-12.core-module-kernel-formattable-markup.txt	11 KB	dww
#12	3397905-12.core-tests-kerneltests-formattable-markup.txt	23.17 KB	dww

Issue fork drupal-3397905

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3 hidden branches

3397905-strict-types-kernel-with-fixes

changes, plain diff MR !6826

Check out this branch for the first time

Check out existing branch, if you already have it locally

3397905-strict-types-kernel-fix-only

changes, plain diff MR !5215

Check out this branch for the first time

Check out existing branch, if you already have it locally

3397905-strict-types-kernel

changes, plain diff MR !5198

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

31 October 2023 at 05:16

mstrelan created an issue. See original summary.

Comment #2

31 October 2023 at 05:18

mstrelan opened merge request !5198

Comment #3

2 November 2023 at 00:15

mstrelan opened merge request !5215

Comment #4

mstrelan commented 2 November 2023 at 00:16

Issue summary:	View changes
Status:	Active	» Needs review

Updated IS to mention the reason for two merge requests. This is ready for review.

Comment #5

smustgrave commented 4 November 2023 at 16:32

Status:

Needs review

» Needs work

Think it needs a rebase, showing 1000+ file changes and says to be unmergable

Comment #6

mstrelan commented 4 November 2023 at 21:16

Status:

Needs work

» Needs review

Let's just review MR !5215 for now. There are indeed over 1000 files changed in the other one.

Comment #7

mstrelan commented 6 November 2023 at 00:45

Title:	Add declare(strict_types=1) to all kernel tests	» Fix strict type errors in kernel tests
Issue summary:	View changes

Comment #8

6 November 2023 at 00:45

mstrelan closed merge request !5198

Comment #9

dww

we/he/they

commented 6 November 2023 at 02:29

Status:

Needs review

» Needs work

Opened 6 MR threads, 3 of which are trivial suggestions, 1 FYI that can be ignored, and 2 places we probably need to continue the sprintf() conversion. Everything else looks great! 😅

Thanks
-Derek

Comment #10

mstrelan commented 6 November 2023 at 03:47

Status:

Needs work

» Needs review

Thanks for the review, have fixed up those issues.

Comment #11

dww

we/he/they

commented 6 November 2023 at 04:34

Status:

Needs review

» Needs work

🏓 😂

Comment #12

dww

we/he/they

commented 6 November 2023 at 04:44

Status	File	Size
new	3397905-12.core-tests-kerneltests-formattable-markup.txt	23.17 KB
new	3397905-12.core-module-kernel-formattable-markup.txt	11 KB

FYI, after checking out the MR branch locally, here's some grep output of remaining instances of FormattableMarkup in any Kernel test code. Some of these are legit. Some might want to be fixed as part of this issue?

egrep -ir FormattableMarkup tests/Drupal/KernelTests > core-tests-kerneltests-formattable-markup.txt
egrep -ir FormattableMarkup core/modules/*/tests/src/Kernel > core-module-kernel-formattable-markup.txt

p.s. Crediting @mstrelan for the work, and myself for reviews.

Comment #13

mstrelan commented 6 November 2023 at 06:16

Status:

Needs work

» Needs review

Fixed issues in #11. I don't think we should do #12 here, the scope is large enough already, unless you feel strongly about any of them.

Comment #14

smustgrave commented 7 November 2023 at 19:09

Status:

Needs review

» Reviewed & tested by the community

Seems threads have been resolved.

Will agree with #13 about expanding the scope so follow up? If not please move back to NW.

Comment #15

mstrelan commented 15 November 2023 at 00:47

Status:

Reviewed & tested by the community

» Needs review

I've removed most of the sprintf calls in favour of string interpolation. In cases where the placeholders use expressions I've mostly stuck with sprintf, and in cases where interpolation leads to awkward escaping of slashes or quotes I've replaced with concatenation.

Comment #16

dww

we/he/they

commented 15 November 2023 at 08:40

Status:

Needs review

» Needs work

cc969fb3 looks much better, thanks!

However, I found a few nits and one assertion where you changed the values being compared and lost test coverage, so back to NW.

Sadly, I ran out of steam at core/tests/Drupal/KernelTests/Core/Entity/ContentEntityCloneTest.php. But I think we're really close, now!

Thanks again,
-Derek

Comment #17

dww

we/he/they

commented 15 November 2023 at 08:42

p.s. Sorry, forgot to explicitly say: absolutely in favor of not expanding the scope any more than necessary in here! 😅 This is already almost too big to read. #12 can be fodder for followups, for sure, we don't need to do that here.

Comment #18

mstrelan commented 15 November 2023 at 12:14

Status:

Needs work

» Needs review

Comment #19

smustgrave commented 15 November 2023 at 15:36

Oh wow the MR has gotten much larger then a day ago haha

Comment #20

dww

we/he/they

commented 15 November 2023 at 18:28

Status:

Needs review

» Reviewed & tested by the community

Thanks for all the work in here, @mstrelan! It's entirely possible I've missed something, but I've probably spent over 3 hours trying to carefully review this monster. 😅 I spot nothing else to complain about. The only open threads are:

A placeholder for where I reviewed that's now obsolete (but which I can't resolve myself).
A question about if/how we should open follow-ups to fix yucky assertion messages that we noticed while working on this.

Neither need to be "solved" before this is committed. Testbot is still happy. GitLab claims it needs a rebase, but I just checked and the latest 5215.diff is applying cleanly to a freshly pulled 11.x core checkout (at commit 54862ffc).

Moving to RTBC to get this in front of the committers.

Thanks again!
-Derek

Comment #21

xjm

she/her

English

commented 15 November 2023 at 21:25

Title:	Fix strict type errors in kernel tests	» [meta] Fix strict type errors in kernel tests
Category:	Task	» Plan
Status:	Reviewed & tested by the community	» Needs work

Nice work on this so far.

I got partway through and started thinking that maybe that comment test should get its own separate issue. Then I looked at the diffstat:
109 files, +630, −683

That's a 1313 LOC change set -- way too large. In general, we should not exceed a total of 400 added and removed lines in order to make our reviews effective, unless the issue is a purely scannable 1:1 replacement that doesn't require thinking about the context. (See Derek's comments above that demonstrate the impact of having an MR that's too large.)

We should split this up into a few smaller scopes of similar change types. It should be straightforward, if a little fiddly, with git add -p, checking out specific files from the bulk branch, or other similar gitifying. So, I'm converting this to a meta.

I think one or more sub-issues could focus on only the FormattableMarkup, which should mostly be converted straight to double-quoted strings for readabillity (mostly not sprintf()). So one or more issues where we're just stripping the FormattableMarkup.
There should be additional issues for tests that are doing complicated ugly things inside FormattableMarkup calls (like the comment tests). The comment test might merit its own dedicated issue to refactor and simplify the test.
Another issue could handle places where defining new local variables improves the readability while FormattableMarkup is removed.
Another issue could handle the addition of string casts.
Another could be dedicated to removing quotes from ints.
Etc. Try splitting it into a few MRs grouped by the type of change that fits, and then check their sizes against the guidelines. We want to target about 100-200 lines changes (for a total of 200-400 added and removed) per MR.

Comment #22

dww

we/he/they

commented 17 November 2023 at 02:37

Issue summary:	View changes
Status:	Needs work	» Needs review

Yes, splitting this is the only way to get it done. 😅 I checked with @mstrelan and agreed I'd help by opening the newly scoped issues, and he'd continue with the git shenanigans for now. I tried to match the proposal in #21 pretty closely:

I'm hoping that by the time we handle all the "easy" conversions in 2 separate issues, that the "everything else" MR won't be that bad. 🙏 If we have to split up #3402294 even further, so be it. 😬

I put this in the remaining tasks, too. Moving to review to make sure the plan sounds okay before we proceed too much further.

Thanks!
-Derek