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

See #3400334: Add declare(strict_types=1) to all test modules

Proposed resolution

Remaining tasks

Review MR !5843

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3402007

Command icon Show commands

Start 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:

About issue forks

Comments

mstrelan created an issue. See original summary.

marvil07 made their first commit to this issue’s fork.

marvil07 opened merge request !5790

mstrelan’s picture

mstrelan commented

Thanks for picking this up @marvil07

Can we change the approach from using rector to using phpcs/phpcbf? See #3400018: Add declare(strict_types=1) to all Functional JavaScript tests 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.

marvil07’s picture

marvil07 commented
Status: Active » Needs work

@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 #3400334: Add declare(strict_types=1) to all test modules.

New commits on new `3402007-strict-types-test-modules-testonly` topic branch

- 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.

marvil07’s picture

marvil07 commented
Issue summary: View changes

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

mstrelan’s picture

mstrelan commented
Issue summary: View changes
mstrelan’s picture

mstrelan commented
Issue summary: View changes

Fixed Drupal\Tests\page_cache\Functional\PageCacheTest

mstrelan opened merge request !5843

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.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

marvil07’s picture

marvil07 commented
Status: Needs work » Needs review

@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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

quietone’s picture

quietone commented

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.

mstrelan’s picture

mstrelan commented

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

quietone’s picture

quietone commented

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

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

mstrelan’s picture

mstrelan commented

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.

mstrelan’s picture

mstrelan commented

Opened #3439800: [pp-1] Standardize location of test modules to address #20.

mstrelan’s picture

mstrelan commented

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/*.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Updated the MR and made the changes for #22

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs followup

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

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review
Issue tags: -Needs followup

Opened followup issue #3461309: Refactor FormTestClickedButtonForm::buildForm

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe all feedback has been addressed

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

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

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

edit: deleted

mstrelan’s picture

mstrelan commented

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

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

  • longwave committed 50496b5e on 10.3.x 
    Issue #3402007 by marvil07, mstrelan, quietone, smustgrave, larowlan:...

  • longwave committed 49283982 on 10.4.x 
    Issue #3402007 by marvil07, mstrelan, quietone, smustgrave, larowlan:...

  • longwave committed b3f2f8de on 11.0.x 
    Issue #3402007 by marvil07, mstrelan, quietone, smustgrave, larowlan:...

  • longwave committed d43e2ba8 on 11.x 
    Issue #3402007 by marvil07, mstrelan, quietone, smustgrave, larowlan:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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