Fix strict type errors in test modules

Thanks for picking this up @marvil07

Can we change the approach from using rector to using phpcs/phpcbf? See 📌 Add declare(strict_types=1) to all Functional JavaScript tests Fixed for example. We can include the rule in core/phpcs.xml.dist in this MR so we can make sure we've got everything.

I also found that for a lot of issues here we need to look at the HTML output to see an error message, as it the underlying error won't present itself in the test output.

@mstrelan, thanks for the feedback.

I will try to take a look at the issue you mention for the next iteration, I did not see your comment until I was about to reply to the issue 😅.

I also found that for a lot of issues here we need to look at the HTML output to see an error message, as it the underlying error won't present itself in the test output.

Indeed, the plan is to identify the tests set with errors, then run them locally, and use the actual test error output URLs produced on the test run as needed, and fix the error.

I opened new MR #5790, intended to point to errors to fix.
As mentioned on its description, the plan is to rebase it to remove the first commit, so that can be done later at 📌 Add declare(strict_types=1) to all test modules Postponed .

New commits on new `3402007-strict-types-test-modules-testonly` topic branch and related new PR #

- 4b73ae24c5 Declare strict types on test modules
- a5b8e85dbd Fix exception on RedirectOnExceptionTest
- 08643a6c68 Fix UncaughtExceptionTest
- 00def53870 No point in return after throwing an exception

The next step is to continue fixing the errors pointed by gitlab CI.

I have changed the approach to use phpcs configuration as the base for modifying the files in bulk.
That produced a few more changes.
I manually modified some of the files to move the declare() after the @file docblock, since it seems like in those cases phpcbf is adding an extra @file section after adding the declare().

I rebased the added changes on top.
And then, I continued with fixing errors as produced after the changes.

At this point four tests are missing to be fixed:

• Drupal\FunctionalJavascriptTests\Core\CsrfTokenRaceTest
• Drupal\Tests\announcements_feed\FunctionalJavascript\AlertsJsonFeedTest
• Drupal\Tests\page_cache\Functional\PageCacheTest
• Drupal\Tests\system\FunctionalJavascript\Form\TriggeringElementTest

Changes now on 3402007-strict-types-test-modules-testonly topic branch

- 7e02f7c188a Add phpcs rule for strict types on test modules
- f98517b9649 Require strict types across test modules
- f56a6768559 Fix exception on RedirectOnExceptionTest
- 6a34dd36f22 Fix UncaughtExceptionTest
- 3adb077ef52 No point in return after throwing an exception
- b06bc731952 Fix errors triggered from SessionTest
- 32688b5d85e Fix errors from FormTest
- 9ddfbea0ea7 Fix error thrown from SecurityAdvisoryTest
- b0eacbada1d Fix error thrown from RenderArrayNonHtmlSubscriberTest
- 5a9015ada2b Fix error thrown from AlertsJsonFeedTest
- 8982568c4e1 CS fix
- 959101d7958 A bit more error handling for WebDriverCurlService
- a175cb41347 One less nesting level on WebDriverCurlService added error handling
- 8266166b3d0 Get the curl error
- 57972a66c5f Fix error thrown from BlockFormInBlockTest
- 63d4287668a Fix errors thrown from EarlyRenderingControllerTest

Fixed Drupal\Tests\page_cache\Functional\PageCacheTest

mstrelan → changed the visibility of the branch 3402007-strict-types-test-modules-testonly to hidden.

mstrelan → changed the visibility of the branch 3402007-strict-types-test-modules-testonly-original to hidden.

Left some comments where I could. Seems to cause a test failure though.

@mstrelan, thanks for the extra test fixes!

@smustgrave, thanks for the extra review, I replied to your comments.

Line numbers

The error is related to line numbers.
One of the fixed tests needed a change of line numbers, because the new naturally declare() affects line numbers.

The new MR contains the changes without the declare() additions, since the task has been split in two steps.

In other words, we need to change line numbers in the test check when we are not adding the declare statements.

I have changed the rebased MR to match line numbers as needed.

using t()

As mentioned on the comment in the MR, we could change multiple tests to avoid using t(), but IMHO that is out of scope of this issue.
More details on my MR comment.

Next steps

Now that tests are passing again, and the threads on the MR has been replied to, I moving the issue back to NR.

Believe feedback has been addressed. Agreed threads about t() are out of scope.

I'm triaging RTBC issues → . I read the IS, the MR and the comments. I didn't find any unanswered questions.

I left comments in the MR, none of which affect the status of this issue. I did not do a thorough review of the MR.

Leaving at RTBC.

@quietone you mentioned in #16 that you left comments in the MR, but I cannot see any comments from you there.

@mstrelan, sorry about that. I have submitted them now.

Left some comments, I think we should keep the intentionally broken code broken

I don't think the pattern we've used to find test modules is adequate. For example, jsonapi has plenty of modules with the pattern tests/modules/json_test_* but we are just looking for */tests/*_test/. Unfortunately not all test modules are found under tests/modules, for example action_form_ajax_test, so we can't use that pattern either.

Opened 📌 Standardize location of test modules Needs review to address #20.

Let's just do the following patterns:

<include-pattern>*/tests/modules/*</include-pattern>
<include-pattern>*/tests/*_test/*</include-pattern>

Anything outside of this can be caught later when we limit to */tests/*.

Updated the MR and made the changes for #22

There one thread from @larowlan mentioning a followup so tagging for that. Rest appears to be addressed.

Opened followup issue 📌 Refactor FormTestClickedButtonForm::buildForm Active

Believe all feedback has been addressed

Super nitpicky but it's cleaner just to declare the type than cast?

I agree with #27 that it would be cleaner, but we can't guarantee it's a string. See my comment on the MR for reasons why.

What's the difference between declaring it in the argument and casting, it works the same?

> $f = new Drupal\Component\Render\FormattableMarkup('markup', []);
= Drupal\Component\Render\FormattableMarkup {#10099
    markup: "markup",
  }

> $callable = fn(string $elements) => $elements;
= Closure(string $elements) {#10093 …2}

> $s = $callable($f);
= "markup"

> gettype($s);
= "string"

Is that allowed with strict types? Thought it would say it expects a string and received a FormattableMarkup.

Yeah, sorry, was wrong there, forgot this was all about strict types :)

In which case as that was the only nit back to RTBC and I can commit.

Backported down to 10.3.x as test-only changes to keep things in sync.

Committed and pushed d43e2ba8f3 to 11.x and b3f2f8de04 to 11.0.x and 492839829a to 10.4.x and 50496b5e22 to 10.3.x. Thanks!

longwave → closed merge request !5843

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