Problem/Motivation

Follow-up to #3547124: Remove system module from kernel tests that don't need it and #3547554: Remove user module from kernel tests that don't need it.

There are various other modules used in kernel tests that are not required. Some of these are because they are already required by the parent class, but others are genuinely not required.

Steps to reproduce

Run this bash script, might take a couple hours to complete. Then review git diff 11.x to see what can be removed and still have passing tests.

#!/bin/bash
for MODULE in core/modules/*; do
  MODULE_NAME=$(basename "$MODULE")

  while IFS= read -r FILE; do
    if grep -q "^    '$MODULE_NAME',\$" "$FILE"; then
      sed -i "/^    '$MODULE_NAME',\$/d" "$FILE"

      if ! ./vendor/bin/phpunit --stop-on-error --stop-on-fail -c core/phpunit.xml.dist "$FILE" >/dev/null 2>&1; then
        git checkout -- "$FILE"
      fi
    fi
  done < <(find core/ -type f -name '*Test.php' | grep -E '/Kernel(/|Tests/)')

done

Proposed resolution

Remaining tasks

It was suggested we could split this in to tests that can have modules removed because they are already installed in a parent class, and tests that don't need the modules installed at all, but since this is a simple bash script there is not enough context to figure that out.

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

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.

mstrelan opened merge request !13582

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs review » Reviewed & tested by the community

Nice clean up! Only changes are the removal of modules from test classes, and the tests still pass. Happy for this to be RTBC

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

I applied the diff and then started the script. I tested the first five files and they all passed tests. I then aborted the script. Is there a reason these are not included in the diff? Are there more files that could be included?

  • core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentCommentSettingsTest.php
  • core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php
  • core/modules/navigation/tests/src/Kernel/NavigationMenuMarkupTest.php
  • core/modules/rest/tests/src/Kernel/Entity/RestPermissionsTest.php
  • core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
mstrelan’s picture

mstrelan commented

I think the script needs to be updated to commit after each module, otherwise later modules can reset the file.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Apologies for that, as per #6 a lot of files were being reverted.

There are two commits worth noting:

  1. 757158e3 Revert DbImportCommandTest - the script reverted some changes to an array that wasn't $modules, so it's worth checking there is nothing else like that. Ideally tests would fail if that were the case.
  2. 62fd03de Revert MigrationPluginListTest - this test is explicitly installing modules that contain Drupal migrations. It's possible that some other tests should have modules installed, whether they are required for the test to pass or not. For example, they could be testing that something does not fail when the module is installed.
smustgrave’s picture

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

echoing #4 seems like a good cleanup.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

This looks lovely, I do like a cleanup. I found 2 migrate tests that should be change. Sadly, I couldn't make a suggestion, GitLab isn't giving me that option. There are lots of files changed here and I relied heavily on the file name to find ones that may need adjustment. After that I checked about a range of files and most often found that the removed modules were loaded in a parent class. And for those that weren't the change to $modules seemed just obvious.

When the changes are made, I think this can go back to RTBC.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community

Pushed two commits for #9, back to RTBC.

  • quietone committed 5a2f583c on 11.3.x 
    Issue #3554484 by mstrelan, danielveza: Remove unused modules from...

  • quietone committed 0bffe2ee on 11.x 
    Issue #3554484 by mstrelan, danielveza: Remove unused modules from...
quietone’s picture

quietone commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

@mstrelan, thanks

Committed to 11.x and cherry-picked to 11.3.x

Thanks!

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

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

Maintainers, credit people who helped resolve this issue.

quietone closed merge request !13582

Status: Fixed » Closed (fixed)

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