Problem/Motivation

Test for aggregator module are scattered around in other modules. We need to consolidate these tests into the modules directory (core/module/aggregator) and into the modules namespace.

This way it will be easier to move the current Core module almost as-is to the Contrib realm

Steps to reproduce

Proposed resolution

Move all tests to the aggregator module.

Remaining tasks

Move all tests to aggregator module.
#3265424: Move migrate related aggregator tests to the module in preparation of removal in d10

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#57 raw_diff_d10-d94-57.txt82.22 KBspokje
#57 3264122-9_4-57.patch116.08 KBspokje
#57 interdiff_54-57.txt1.49 KBspokje
#54 3264122-9_4-54.patch114.43 KBspokje
#54 raw_diff_d10-d94-54.txt83.61 KBspokje
#53 3264122-9_4-52.patch114.42 KBspokje
#53 raw_diff_d10-d94-52.txt83.6 KBspokje
#51 3264122-9_4-51.patch119.94 KBspokje
#51 raw_diff_d10_d94.txt113.9 KBspokje
#22 Screenshot 2022-02-23 at 10.19.18 AM.png90.37 KBankithashetty
#17 raw_diff_1812-1852.txt17.54 KBspokje
Show 8 more files

Issue fork drupal-3264122

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

Toggle MR activity (91 comments)

Spokje created an issue. See original summary.

Spokje opened merge request !1812

Issue #3264122: Move all aggregator tests to the module in preperation of removal in d10
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Title: Move all aggregator tests to the module in preperation of removal in d10 » Move all aggregator tests to the module in preparation of removal in d10
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Issue summary: View changes
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I will start looking into all the migration tests. It is a bit tricky because aggregator is used to do some extra checking on the migration state and that will have to move to another module. I hope to make more informed comments as a I make commits,

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Thanks @quietone, I've been looking at the non migrate_drupal_ui migration related failing tests when I remove the complete core/modules/aggregator directory on top if the changes here on 10.0.x-dev. but am to scared/ignorant to touch them.

They definitely need the touch of a Migration MasterMind.

Also I've left (quite) a few remarks in the MR on the code>migrate_drupal_ui changes that I've made. If you could comment on those as well that would be great.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue tags: +Needs followup

Just updated the functional tests for the aggregator module.

The migrate fixtures are copied to aggregator module so that the aggregator tests are independent of future changes to the fixtures.

A followup could be made in aggregator to add testing of aggregator Id conflicts. This is not tested in core so is a new test. I did check this locally and it works as expected.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Started on the Kernel tests. This changes the aggregator migration Kernel tests to use the test fixture in aggregator.

There are more Kernel tests to handle.

$ grep -Ir aggregator core/modules/ | grep -v fixture | grep -v .js | grep -v modules/aggregator | grep -v modules/migrate_drupal_ui | grep -i migrat | grep Test.php | awk -F: '{print $1}' | sort -u | nl
     1  core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php
     2  core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
     3  core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockContentTranslationTest.php
     4  core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
     5  core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7AuditIdsTest.php
     6  core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php

The size of this MR is increasing and I wonder if we should make it easier to review by breaking up into sensible parts. At least the migration work can be done in a separate issue.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

For some of the tests I put in an @todo for the cases where aggregator can be removed when the module if removed from core. They should has an issue number but don't. Is there an issue for the actual removal?

Assuming what I have done is correct and tests are passing, the task remaining is to figure out what to do for d6_block and d7_block migrations. Perhaps, in a few hours, I can pick this up again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

#3264120: Remove aggregator module and our dependency on Laminas Feed is the removal issue.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes

@catch, thanks, I was going to find look properly for that link today.

On further thought, I think it will help reviewers if the migration work is moved to a separate issue, so I have added a child issue. I've reverted the migrate related changes here.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Title: Move all aggregator tests to the module in preparation of removal in d10 » [PP-1] Move all aggregator tests to the module in preparation of removal in d10
Status: Active » Postponed
Related issues: +#3265424: Move migrate related aggregator tests to the module in preparation of removal in d10

Adding above mentioned issue []#3265424 as related (Thanks @quietone for the excellent work done on migrations). Unsure if this should be postponed on that issue.

Pretty sure this should go into 10.0.x-dev first (See #3263654-28: Move all HAL tests to the module in preparation of removal in D10).

Not doing the admin for that currently.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Status: Postponed » Active

Reverting unplanned status change

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Title: [PP-1] Move all aggregator tests to the module in preparation of removal in d10 » Move all non migration aggregator tests to the module in preparation of removal in d10

@Spokje, this doesn't need to be postponed on #3265424: Move migrate related aggregator tests to the module in preparation of removal in d10.

Updating the title to make it clear that this isn't doing the migration tests. And removing the [PP-1] from the title because it has been set to active.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Version: 9.4.x-dev » 10.0.x-dev

Thanks @quietone.

Upped version to 10.0.x-dev

Spokje opened merge request !1852

Issue #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
StatusFileSize
new raw_diff_1812-1852.txt17.54 KB

MR!1812 -> 9.4.x-dev
MR!1852 -> 10.0.x-dev

Attached a raw diff between the two MRs.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Assigned: spokje » Unassigned
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

This seems to contain moving Help Topics as well as tests? Shouldn't that be a separate scope?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Needs work

FNW for #19.

BTW, it's preferred to rebase these MRs against HEAD instead of merging HEAD to them. :)

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

> BTW, it's preferred to rebase these MRs against HEAD instead of merging HEAD to them. :)

Must have missed that somehow in the documentation, could you point me to where I can find that?

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer and at Specbee commented
Issue summary: View changes
StatusFileSize
new Screenshot 2022-02-23 at 10.19.18 AM.png90.37 KB

There is a message on the MR page saying the source branch must be rebased onto the target branch.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer and at Specbee commented

Just rebased the MR with the latest 10.0.x branch.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Thanks @ankithashetty for pointing me to (at least the GitLab) documentation, unsure if the MR needed a rebase already.

@xjm:

This seems to contain moving Help Topics as well as tests? Shouldn't that be a separate scope?

Can we have a section added to the Update Drupal deprecation policy about the correct procedure to deprecating/removing core modules and themes, as marked as a "Remaining task" in the IS of #3135100: [policy and meta] Provide a proper mechanism for deprecating modules and themes?

I've spent (way too much) time on trying to get quickedit, hal and aggregator deprecated/removed, and, during the last two weeks, only seen all three of them moving "forwards" faster than Michael Jackson doing the moonwalk, meaning that they are on a forward track, but they're actually moving backwards and are only getting postponed on more and more on sub-issue blockers.

Since we're obviously doing something (kinda) new here, bumping into unexpected things is to be expected.
Adding extra blockers on it due to (at least me) being unsure about to need for help topics, moving core issues about the modules to the Contrib queue, altering textual references like for example in core.api.php etcetera, needing a separate issue or not, would be a shame IMHO.

IMHO Looking at the cut-off date of (I actually don't know for sure) when 9.4.0 will be released, it's getting more and more uncertain if we can make it with the modules that started the journey Core => Contrib already.

A clear What-To-Do-Beside-Break-The-Glass-In-Case-Of-Emergency-And-Core-Module/Theme-Deprecation would save everybody a lot of time (again IMHO :)

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

I agree with you there @Spokje. Its hard work and understand your frustration in running into blocker after blocker that you could not have foreseen.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Thanks @bbrala.

Personal frustration is temporary, missing the deadline of 9.4.0 is forever (or at least until another x.y.0 deadline).

I'm fully OK with being frustrated for a tiny bit, but it would be a shame to miss deadlines because of unclear admin on creating sub-issues.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Sorry for the noise, but prevent even more coming in, there's a Slack thread for "Deprecating/Removing Core Modules for Dummies" right here: https://drupal.slack.com/archives/C014CT1CN1M/p1645607618306019.

Since this issue isn't about that guidance, let's discuss it down there for now.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

This seems to contain moving Help Topics as well as tests? Shouldn't that be a separate scope?

fwiw I think it's worth doing that in a separate issue - in this case it's just git mv so it'll be a very easy RTBC and commit, then mean a smaller diff to review here. If it was just one or two tests + help topics and just moving things around, then could probably get away with a single issue though.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Status: Needs work » Needs review

Thanks @quietone for the moving-the-move of the aggregator related help topics to its own issue.

Rebased once again and putting back to Needs Review.

Spokje closed merge request !1812

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented

Copied over my 2 remarks from the 9.4.x-MR and closed that one, since it was outdated and we need to go into 10.0.x-dev first anyway.

This is (Yet Again) a massive MR to review, but most of the changes are Views related, where we "simply" changed the signature of one function setUp() which cascades into a snowball-from-hell quite quickly.

We did the same for the deprecation/removal of hal BTW, so no rocket scientist were harmed (or even used) during the creation of this MR.

The actual test-moving-around is quite small, since the migration issue #3265424: Move migrate related aggregator tests to the module in preparation of removal in d10 is doing a lot of the heavy lifting for that.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented

Unrelated test-failure is being handled here: #3267124: Temporarily skip failing tests and even appears to pop up in the current HEAD of 10.0.x-dev at the time of testing: https://www.drupal.org/pift-ci-job/2330048.

Keeping this on NR.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Parent issue: » #3188549: [Meta] Tasks to deprecate both Aggregator and our dependency on Laminas Feed
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

The change to modules/views/tests/src/Functional/ViewTestBase.php does not look necessary. Can somebody explain why it is in?

  protected function setUp($import_test_views = TRUE): void {
  protected function setUp($import_test_views = TRUE, $test_views_module = ['views_test_config']): void {
    parent::setUp();
    if ($import_test_views) {
      ViewTestData::createTestViews(static::class, ['views_test_config']);
      ViewTestData::createTestViews(static::class, $test_views_module);
    }
  }
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Just committed the patches for migration-related tests and help_topics, so this issue should hopefully have a clean run now.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented

@daffie:

The change to modules/views/tests/src/Functional/ViewTestBase.php does not look necessary. Can somebody explain why it is in?

This is because of the moving of the view test_style_opml to the aggregator module in core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_style_opml.yml.

Since it's now not in the test_views module anymore, it's not automagically past along as a default value to the setUp() method. Changing that cascaded in a Whole Lot Of Rosie Changes in setUp() for subclasses of ViewTestBase.

We did something similar for the hal module in its MR.

Hope that makes any sense?

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Status: Needs work » Needs review

Rebased MR after relevant MRs/patches were committed (thanks @catch) and tried to answer @daffie's question (Thanks for the review)

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Assigned: Unassigned » spokje
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Assigned: spokje » Unassigned

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

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
CreditAttribution: taran2l at Lemberg Solutions commented

This MR cannot be rebased automatically, so let's wait it to pass and then I'll do it manually.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
CreditAttribution: taran2l at Lemberg Solutions commented
Status: Needs work » Needs review
taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
CreditAttribution: taran2l at Lemberg Solutions commented

OK, after rebase run has a single (I believe a random) failure ... keeping the status

  • catch’s picture
    Merge request !1852 catch updated ✗ unresolved

    The diff is huge, but a large proportion of it is multiple changes like this.

    Since we've already split the aggregator removal scope into multiple issues, I think it's fine to handle this in this patch even though it doesn't explicitly mention aggregator.

  • catch’s picture
    Merge request !1852 catch updated ✗ unresolved

    Was wondering if it was really OK to remove this, until I realised it was doing precisely nothing given the next line.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

OK I reviewed this line by line.

It's a massive diff. Most of it is changes to views tests (almost all the same change) due to the way test views are handled after the removal of the OPML test view from views into aggregator. I was nearly tempted to ask for that to be moved to a different issue, but we've split the aggregator removal scope up a lot already, and since there's not that many other changes in the patch, I think it's fine here.

There are probably things we can do to further clean-up some of these tests, but that's all out of scope here which is about removing dependencies. So... RTBC for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 39b12c4 and pushed to 10.0.x. Thanks!

Do we want to try to backport some of the changes here to 9.4.x to make test maintenance easier?

  • alexpott committed 39b12c4 on 10.0.x 
    Issue #3264122 by Spokje, Taran2L, ankithashetty, quietone, catch, xjm,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Fixed » Reviewed & tested by the community

I think it would be good to backport the entire patch to 9.4.x if we can, so that the contrib and 9.4.x versions of aggregator are as close as possible.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented

I would say an absolute YES! to backporting to 9.4.x-dev.

This to keep:

- Inline with previously deprecated/removed Core modules where we did this (HAL, QuickEdit)
- Make test maintenance easier.
- Make backporting in general easier (remember the SA issue on HAL anybody?)
- Make the Contrib incarnation of aggregator as close as possible to the remaining core one

Would that mean a new MR/patch or does the current one also apply on 9.4.x-dev?

INSTA-EDIT: Too slow...

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Assigned: Unassigned » spokje

Would that mean a new MR/patch or does the current one also apply on 9.4.x-dev?

It doesn't, 9.4.x-dev patch incoming.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
StatusFileSize
new raw_diff_d10_d94.txt113.9 KB
new 3264122-9_4-51.patch119.94 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 3264122-9_4-51.patch, failed testing. View results

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
StatusFileSize
new raw_diff_d10-d94-52.txt83.6 KB
new 3264122-9_4-52.patch114.42 KB
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
StatusFileSize
new raw_diff_d10-d94-54.txt83.61 KB
new 3264122-9_4-54.patch114.43 KB

*sigh* There's always one that escapes...

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

Straight re-roll as far as I can see

+++ 3264122-9_4-54.patch	2022-03-17 20:00:25.581070100 +0100
@@ -442,87 +442,8 @@
 +}
-diff --git a/core/modules/aggregator/tests/src/Unit/BubbleableMetadataTest.php b/core/modules/aggregator/tests/src/Unit/BubbleableMetadataTest.php
-new file mode 100644
-index 0000000000000000000000000000000000000000..0189486c12222bbb3b2b1819d9bef41dda0cf7ad
---- /dev/null
-+++ b/core/modules/aggregator/tests/src/Unit/BubbleableMetadataTest.php
-@@ -0,0 +1,73 @@

This from the raw diff looks like it might need adding to the 9.4.x patch (so it doesn't appear in the diff between the 9.4 and 10.x changes).

Otherwise looks good to me.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff_54-57.txt1.49 KB
new 3264122-9_4-57.patch116.08 KB
new raw_diff_d10-d94-57.txt82.22 KB

Thanks @catch, that one indeed didn't make it through the manual re-add new files check :/

*sigh* There's always one two that escapes...

Added in attached patch, leaving it on RTBC.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje at Chromatic commented
Status: Needs review » Reviewed & tested by the community

leaving it on RTBC.

he said whilst browser put it back to NR

  • alexpott committed ed58a15 on 9.4.x 
    Issue #3264122 by Spokje, Taran2L, ankithashetty, quietone, catch, xjm,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed ed58a15 and pushed to 9.4.x. Thanks!

Spokje closed merge request !1852

Status: Fixed » Closed (fixed)

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

berdir’s picture

berdir
Primary language German
Location Switzerland
CreditAttribution: berdir commented

The ViewTestBase change is really awkward, that causes a breaking change in test modules, I don't really see right now how a test class would be able to support both D10 and D9 with that constructor change. Going to to try and remove the constructor entirely, but it's a bit awkward.

Edit: I might be wrong, I think you can have more extra arguments, just not less? Still a bit awkward, but less of an issue then.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue tags: -Needs followup

I added the needs followup tag and I don't think it is necessary to add an ID conflict test to aggregator, it is the job of migrate to find ID conflicts not aggregator. There was other discussion about a followup in this discussion in the MR. The reply from @Spokje implies a follow up is not needed. I am removing the tag.