Problem/Motivation

Part of #3524379: [meta] Remove usage of history module from comment module

Once we've moved everything out of comment module we need to remove history module from CommentTestBase. This will likely require some additional test refactoring.

Steps to reproduce

Proposed resolution

  • Remove history module from CommentTestBase::$modules
  • See what tests fail due to this dependency
  • Move or split tests to history module

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3546095-nr-bot_ob3t8_ct.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3546095

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

mstrelan’s picture

mstrelan commented
Title: [PP-4] Remove history module from CommentTestBase::$modules » Remove history module from CommentTestBase::$modules
Status: Active » Needs review

Actually we don't need to postpone this, it can happen first, or at any time.

smustgrave’s picture

smustgrave commented

Since you already reviewed are those the changes needed here? Haha

mstrelan’s picture

mstrelan commented

From my perspective, history module is going to contrib where the maintainer can do whatever they want. I'd rather do the minimal amount to deprecate it. The suggested changes would make the tests make more sense on their own, but I didn't want to deviate too much from the original tests to keep the scope down.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

Left a couple of review commments. Just some question around the moving the token test coverage to the history module

mstrelan’s picture

mstrelan commented

Thanks @danielveza, I've responded on the MR.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

All the explanations make sense to me!

This is RTBC from me, there is a couple of self review thoughts in there so I'll leave it in NR for the moment, but if you're happy with the current state @mstrelan this can be moved to RTBC

smustgrave’s picture

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

Believe feedback on this one has been addressed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3546095-nr-bot_ob3t8_ct.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.

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

quietone’s picture

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

Simple conflict due to #3547849: Add #[RunTestsInSeparateProcesses] attribute to all Functional/FunctionalJavascript tests. I've rebased and am restoring RTBC.

catch’s picture

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

Looks like there's still some unresolved feedback on the MR from @mstrelan.

quietone’s picture

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

Rebase again, due to #3542540: Deprecate comment libraries and move to history module. Simple conflict in core/modules/history/tests/src/Functional/CommentCSSTest.php. I am restoring RTBC.

  • catch committed 06cdf971 on 11.x 
    Issue #3546095 by mstrelan, danielveza, quietone: Remove history module...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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.

catch closed merge request !13235

Status: Fixed » Closed (fixed)

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