Problem/Motivation

Follow-up to #2943436: Review all tests where system module is the only dependency

Steps to reproduce

Proposed resolution

Use this bash script to find and replace possible candidates. If the test fails it will be reverted. If it passes, we can review and commit the change.

find core/ -type f -name '*Test.php' \
  | grep -E '/Kernel(/|Tests/)' \
  | while read -r FILE; do
    if grep -q "^    'system',\$" "$FILE"; then
      sed -i "/^    'system',\$/d" "$FILE"

      if ! ./vendor/bin/phpunit --stop-on-error --stop-on-fail -c core/phpunit.xml.dist "$FILE"; then
        echo "Test failed for $FILE, reverting..."
        git checkout -- "$FILE"
      else
        echo "Test passed for $FILE"
      fi
    fi
  done

Remaining tasks

Repeat for other modules

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3547124

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:

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

Title: Remove system module from tests that don't need it » Remove system module from kernel tests that don't need it
Issue summary: View changes
Status: Active » Needs review

Updated to kernel tests only. Functional tests get system module automatically, but some tests should explicitly require it.

mstrelan’s picture

Issue summary: View changes
nicxvan’s picture

The other issue was only about tests that has only the one dependency.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Assuming there would be failures if this caused issues.

mstrelan’s picture

Re #5 does that matter? As per #6 these tests don't need system module installed, or in some cases it's already installed in a parent class. IMHO tests should seek to do the minimum amount of setup as possible to make the test pass.

nicxvan’s picture

No sorry!

I was just commenting based on #2943436-35: Review all tests where system module is the only dependency that the other issue was scoped to singletons, I think this is great cleanup and I agree with 7, any reduction we can do is a win.

  • catch committed 7b22a40f on 11.x
    Issue #3547124 by mstrelan: Remove system module from kernel tests that...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Yeah this makes sense. Looking at the diff I wonder if it's worth checking the same thing for 'user'.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

mstrelan’s picture

Sure, I opened #3547554: Remove user module from kernel tests that don't need it but have some concerns that I've raised on the MR.

Status: Fixed » Closed (fixed)

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