Problem/Motivation

Found while working on #3547124: Remove system module from kernel tests that don't need it

\Drupal\Tests\system\Kernel\Scripts\DbImportCommandTest installs a bunch of modules, but not their schema, then imports a db dump. What's the point installing the modules in the first place?

FWIW the test was added in #2550291: Improve and generalize database dump tools

Steps to reproduce

NA

Proposed resolution

Remove the $modules property

Remaining tasks

MR for 11.2

User interface changes

NA

Introduced terminology

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

CommentFileSizeAuthor
#5 3547128-nr-bot__3k5p7ma.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3547128

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 !13290

mstrelan’s picture

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

smustgrave commented
Title: Why does DbImportCommandTest install modules? » DbImportCommandTest unnecessarily installs modules
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative, +Bug Smash Initiative

Nice find! Cleaned up the title some but tests are still green so definitely not needed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3547128-nr-bot__3k5p7ma.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +no-needs-review-bot

MR applies cleanly, bot is drunk

quietone’s picture

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

Sorry, diff no longer applies.

mstrelan’s picture

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

OK, I was trusting the MR overview page saying it merges cleanly. I tested manually and can confirm it applies with patch -p1 but not with git apply.

$ git fetch && git reset --hard origin/11.x
HEAD is now at 5cbac86546d #3548596: Add 'View' to operations links where available feat: Add 'View' to operations links where available
$ curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | git apply
error: patch failed: core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php:16
error: core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php: patch does not apply
$ curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | patch -p1
patching file core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
Hunk #1 succeeded at 17 with fuzz 1 (offset 1 line).

Have rebased and force pushed now.

  • quietone committed 09f98dee on 11.x 
    Issue #3547128 by mstrelan, smustgrave: DbImportCommandTest...
quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

@mstrelan, thanks for the details.

Committed to 11.x.

The change did not cherry-pick though so not committed to 11.2.x and neither did applying the patch using curl -s https://git.drupalcode.org/project/drupal/-/merge_requests/13290.diff | patch -p1. Seeting to needs work for an MR on 11.2.x

mstrelan opened merge request !13353

mstrelan’s picture

mstrelan commented
Version: 11.x-dev » 11.2.x-dev
Status: Needs work » Needs review

Have opened an MR for 11.2.x, although not sure if should backport this, since we didn't do the same for #3547124: Remove system module from kernel tests that don't need it, which is the source of the conflict.

quietone closed merge request !13353

quietone closed merge request !13290

quietone’s picture

quietone commented
Version: 11.2.x-dev » 11.x-dev
Status: Needs review » Fixed

@mstrelan, thanks. Yes, we should skip committing this to 11.2.x. I apologize for wasting your time.

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.

Status: Fixed » Closed (fixed)

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