[meta] Fix strict type errors in kernel tests

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

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

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

mstrelan → closed merge request !5198

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

Thanks for the review, have fixed up those issues.

🏓 😂

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.

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.

Seems threads have been resolved.

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

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.

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

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.

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

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:

1. A placeholder for where I reviewed that's now obsolete (but which I can't resolve myself).
2. 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

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.

1. 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.
2. 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.
3. Another issue could handle places where defining new local variables improves the readability while FormattableMarkup is removed.
4. Another issue could handle the addition of string casts.
5. Another could be dedicated to removing quotes from ints.
6. 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.

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:

1. 📌 Fix strict type errors: Convert FormattableMarkup to strings (simple replacement) in core/tests/Drupal/KernelTests/* Active
2. 📌 Fix strict type errors: Convert FormattableMarkup to strings (simple replacement) in core/modules/*/tests/src/Kernel/* Active
3. 📌 Fix strict type errors: Convert FormattableMarkup to strings (complex replacement) in core Kernel tests Active
4. 📌 Fix strict type errors in kernel tests: Add (string) casts where needed Active
5. 📌 Fix strict type errors in kernel tests: Do not quote integers Active

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

Adding 📌 Fix strict type errors in CommentFieldAccessTest Active since yeah, that 1 test is nearly 150 lines of diff...

Added 📌 Fix usages of AssertContentTrait and stop recommending FormattableMarkup Active as another child.

mstrelan → closed merge request !5215

Opened a mega MR of all related issues to see what's left to fix.

After reviewing 📌 Fix strict type errors in AssertContentTrait Needs work , should we have a follow up to convert remaining incorrect usages of FormattableMarkup in Kernel tests?

All child issues are done, I think we can close this unless there is anything I missed.

quietone → closed merge request !6826

Automatically closed - issue fixed for 2 weeks with no activity.