Handle migration tests for removing tracker [#3267314]

Problem/Motivation

Make sure the functional tests in migrate_drupal_ui work without tracker installed and that the migration tests in tracker still work when tracker is removed from core.

Steps to reproduce

Proposed resolution

Add @todo where needed to remove tracker references in the removal issue
Copy the d7 fixture to tracker for the tracker tests and update cspell.json to ignore it
Create a version of NoMultilingualReviewPageTest to confirm that Tracker will be upgrade when enabled on the destination
Do not create an Upgrade7Test in tracker because there are no entities to count.

Remaining tasks

Patch
Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#18	3267314-18.patch	934.89 KB	quietone
#18	10.0.x: PHP 8.1 & MySQL 5.7 28,457 pass PHP 8.1 & MySQL 5.7 28,353 pass, 70 fail 9.5.x: PHP 8.1 & MySQL 5.7 29,918 pass
#18	diff-16-18.txt	2.12 KB	quietone
#16	3267314-16.patch	934.9 KB	quietone
#16	10.0.x: PHP 8.1 & MySQL 5.7 28,519 pass
#16	interdiff-14-16.txt	5.45 KB	quietone
#14	3267314-14.patch	930.01 KB	quietone
#14
#14	interdiff-11-14.txt	694 bytes	quietone
#11	3267314-10.patch	929.93 KB	quietone
#11	10.0.x: PHP 8.1 & MySQL 5.7 28,408 pass, 1 fail 9.4.x: PHP 8.0 & MySQL 5.7 29,590 pass, 1 fail
#11	3267314-review-only-10.patch	9.47 KB	quietone
#11
#9	3267314-9.patch	1.73 MB	quietone
#9	10.0.x: PHP 8.1 & MySQL 5.7 28,408 pass, 1 fail
#9	diff-5-9.txt	846 bytes	quietone
#8	3267314-5-review-only.patch	9.45 KB	quietone
#8
#5	3267314-5.patch	1.73 MB	quietone
#5	10.0.x: PHP 8.1 & MySQL 5.7 28,425 pass, 2 fail PHP 8.1 & MySQL 5.7 Patch Failed to Apply 9.4.x: PHP 8.0 & MySQL 5.7 29,563 pass, 4 fail
#5	interdiff-4-5.txt	6.29 KB	quietone
#4	3267314-4.patch	1.74 MB	quietone
#4	10.0.x: PHP 8.1 & MySQL 5.7 28,429 pass, 2 fail
#4	interdiff-2-4.txt	1.35 KB	quietone
#2	3267314-2.patch	1.74 MB	quietone
#2	10.0.x: PHP 8.1 & MySQL 5.7 28,429 pass, 2 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

2 March 2022 at 10:04

quietone created an issue. See original summary.

Comment #2

quietone CreditAttribution: quietone at PreviousNext commented 3 March 2022 at 07:11

Issue summary:	View changes
Status:	Active	» Needs review

File	Size
3267314-2.patch	1.74 MB
10.0.x: PHP 8.1 & MySQL 5.7 28,429 pass, 2 fail

Let's try this.

Comment #3

3 March 2022 at 08:07

Status:

Needs review

» Needs work

The last submitted patch, 2: 3267314-2.patch, failed testing. View results

Comment #4

quietone CreditAttribution: quietone at PreviousNext commented 3 March 2022 at 09:20

File	Size
interdiff-2-4.txt	1.35 KB
3267314-4.patch	1.74 MB
10.0.x: PHP 8.1 & MySQL 5.7 28,429 pass, 2 fail

Copied the wrong file. Start again for NoMultilingualReviewPageTest in tracker.

Comment #5

quietone CreditAttribution: quietone at PreviousNext commented 3 March 2022 at 23:12

File	Size
interdiff-4-5.txt	6.29 KB
3267314-5.patch	1.73 MB
10.0.x: PHP 8.1 & MySQL 5.7 28,425 pass, 2 fail PHP 8.1 & MySQL 5.7 Patch Failed to Apply 9.4.x: PHP 8.0 & MySQL 5.7 29,563 pass, 4 fail

File

Size

interdiff-4-5.txt

6.29 KB

3267314-5.patch

1.73 MB

9.4.x:
PHP 8.0 & MySQL 5.7 29,563 pass, 4 fail

I think the test is failing because a directory, which is empty, is not created. This should fix that and I've trimmed down the ReviewPage test to the bare minimum.

Comment #6

quietone CreditAttribution: quietone at PreviousNext commented 3 March 2022 at 23:12

Status:

Needs work

» Needs review

Comment #7

4 March 2022 at 00:10

Status:

Needs review

» Needs work

The last submitted patch, 5: 3267314-5.patch, failed testing. View results

Comment #8

quietone CreditAttribution: quietone at PreviousNext commented 4 March 2022 at 04:23

Status:

Needs work

» Needs review

File	Size
3267314-5-review-only.patch	9.45 KB

Adding a review only patch that removes the test fixture from the patch in #5.

The failing tests are random javascript test fails, so setting this to NR.

Comment #9

quietone CreditAttribution: quietone at PreviousNext commented 17 March 2022 at 00:38

File	Size
diff-5-9.txt	846 bytes
3267314-9.patch	1.73 MB
10.0.x: PHP 8.1 & MySQL 5.7 28,408 pass, 1 fail

Rereoll

Comment #10

17 March 2022 at 01:12

Status:

Needs review

» Needs work

The last submitted patch, 9: 3267314-9.patch, failed testing. View results

Comment #11

quietone CreditAttribution: quietone at PreviousNext commented 17 March 2022 at 01:39

Status:

Needs work

» Needs review

File	Size
3267314-review-only-10.patch	9.47 KB

3267314-10.patch	929.93 KB
10.0.x: PHP 8.1 & MySQL 5.7 28,408 pass, 1 fail 9.4.x: PHP 8.0 & MySQL 5.7 29,590 pass, 1 fail

Failure was 1) Drupal\Tests\layout_builder\FunctionalJavascript\ContextualLinksTest::testContextualLinks

This just reduces the size of the tracker fixture by removing things like translations and lots of fields. The fixture still works on a d7 site.

Comment #12

17 March 2022 at 02:51

Status:

Needs review

» Needs work

The last submitted patch, 11: 3267314-10.patch, failed testing. View results

Comment #13

quietone CreditAttribution: quietone at PreviousNext commented 17 March 2022 at 03:59

Status:

Needs work

» Needs review

The failure on 10 was Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove

Comment #14

quietone CreditAttribution: quietone at PreviousNext commented 22 March 2022 at 03:10

File	Size
interdiff-11-14.txt	694 bytes
3267314-14.patch	930.01 KB

Adding an @todo. No need to run tests again for a comment.

Comment #15

danflanagan8

English

St. Louis, US

CreditAttribution: danflanagan8 at Horizontal commented 22 March 2022 at 15:51

Status:

Needs review

» Needs work

@quietone, I think I found a couple things that need an update.

1. After applying the patch in #14 I still see tracker referenced in MigrateDrupal6AuditIdsTest and MigrateDrupal7AuditIdsTest. I think we probably need to add the standard @todo for those.

2. I think that we can remove the cspell line from Drupal\Tests\tracker\Functional\Migrate\ReviewPageTest. That appears to be a copy/paste relic.

3. Is MigrateTrackerSettingsTest completely new test coverage? That's not a problem exactly, but it's driving me nuts that I can't figure out where that came from.

Looking good! I'm going to set back to NW.

Comment #16

quietone CreditAttribution: quietone at PreviousNext commented 23 March 2022 at 00:21

Status:

Needs work

» Needs review

File	Size
interdiff-14-16.txt	5.45 KB
3267314-16.patch	934.9 KB
10.0.x: PHP 8.1 & MySQL 5.7 28,519 pass

@danflanagan8, thanks.

#1. I was more careful in my checking for 'tracker' this time and have added more @todo, including the files mentioned.
#2. Done
#3. It is not a new test. It is testing d7_tracker_settings, #2353777: Migrate Tracker module variables in D7.

The tests of the ReviewPage in migrate_drupal_ui are still testing with Tracker enabled and just have @todos to remove tracker later. Ideally, Tracker would not be installed in these tests but I am not convinced the extra work to do that is needed. The ReviewPage relies on the state files, and the combination of state files and the possible combinations is well tested.

Comment #17

danflanagan8

English

St. Louis, US

CreditAttribution: danflanagan8 at Horizontal commented 23 March 2022 at 13:42

Thanks for the updates, @quietone, especially the note on d7_tracker_settings. My question must have sounded crazy now that I see that file is not new, just very slightly modified. D'oh!

At this point I am noticing an inconsistency between this issue and the issue to remove aggregator. In that issue, aggregator got uninstalled in the ReviewPage tests and it got moved from the getAvailablePaths to getMissingPaths. And there is no @todo to remove "Aggregator" from getMissingPaths. I recently defended this in #3264120-17: Remove aggregator module and our dependency on Laminas Feed:

There are several mentions of "Aggregator" in migrate_drupal_ui tests, but this is only in the context of MigrateUpgradeTestBase::getMissingPaths. That is correct and does not need any attention.

Your latest patch though adds @todos to remove tracker from getAvailablePaths but there's nothing about adding them to getMissingPaths. And the RDF patch actually adds @todos to remove RDF from getMissingPaths.
(RDF issue: #3267513: Handle migration tests for removing RDF)

So now I'm not sure which of the approaches is correct. I had assumed aggregator would be the model, but maybe not. Can you clarify for me?

Comment #18

quietone CreditAttribution: quietone at PreviousNext commented 25 May 2022 at 00:16

File	Size
diff-16-18.txt	2.12 KB
3267314-18.patch	934.89 KB
10.0.x: PHP 8.1 & MySQL 5.7 28,457 pass PHP 8.1 & MySQL 5.7 28,353 pass, 70 fail 9.5.x: PHP 8.1 & MySQL 5.7 29,918 pass

File

Size

diff-16-18.txt

2.12 KB

3267314-18.patch

934.89 KB

10.0.x:
PHP 8.1 & MySQL 5.7 28,457 pass
PHP 8.1 & MySQL 5.7 28,353 pass, 70 fail

9.5.x:
PHP 8.1 & MySQL 5.7 29,918 pass

I had assumed aggregator would be the model

Unfortunately that is not true. How each module behaves depends on its status in the standard profile, the source database, and if there are related migrations. We discovered with aggregator that the block migration will migrate all blocks, including aggregator ones. So we left those blocks in the source database since the migration needs to handle that case properly, we should test it. That make aggregator a special case.

Patch needed a reroll.

Comment #19

25 May 2022 at 01:13

Status:

Needs review

» Needs work

The last submitted patch, 18: 3267314-18.patch, failed testing. View results

Comment #20

quietone CreditAttribution: quietone at PreviousNext commented 25 May 2022 at 13:49

Status:

Needs work

» Needs review

Unrelated fail, Drupal\Tests\media_library\FunctionalJavascript\WidgetOEmbedTest::testWidgetOEmbedAdvancedUi.

Comment #21

Spokje

Dutch

CreditAttribution: Spokje as a volunteer commented 18 June 2022 at 17:24

Status:

Needs review

» Reviewed & tested by the community

LGTM

Comment #22

Spokje

Dutch

CreditAttribution: Spokje as a volunteer commented 18 June 2022 at 17:27

Priority:	Normal	» Major
Issue tags:		+blocker

Since this is blocking #3261452: [11.x] Remove tracker module from core, which has priority major, also bumping this one to major.

Comment #23

20 June 2022 at 19:30

catch committed 12762b4 on 10.0.x

Issue #3267314 by quietone, danflanagan8: Handle migration tests for...

catch committed 6166cd0 on 9.5.x

Issue #3267314 by quietone, danflanagan8: Handle migration tests for...

Comment #24

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 20 June 2022 at 19:30

Version:	10.0.x-dev	» 9.5.x-dev
Status:	Reviewed & tested by the community	» Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.5.x, thanks!

Comment #25

Spokje

Dutch

CreditAttribution: Spokje as a volunteer commented 21 June 2022 at 07:04

Priority:	Major	» Critical
Status:	Fixed	» Needs work

I have _no_ clue how this happened, but by the look of https://www.drupal.org/pift-ci-job/2408230 the commit to 10.0.x broke HEAD.

Reopening and marking as Critical.

Comment #26

21 June 2022 at 14:12

catch committed add472c on 10.0.x

Revert "Issue #3267314 by quietone, danflanagan8: Handle migration tests...

Comment #27

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 21 June 2022 at 14:12

Status:

Needs work

» Needs review

I've reverted this from 10.0.x, left it in 9.5.x until we know whether we need to redo the patch here or it's an unlucky interaction with another patch.

Comment #28

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 21 June 2022 at 14:14

Sent the patch for a fresh run against 10.0.x

Comment #29

Spokje

Dutch

CreditAttribution: Spokje as a volunteer commented 21 June 2022 at 15:28

"Fun" stuff, the patch now fails similar in the way HEAD failed when the patch was applied, weirdly on it's previous runs, the last one being literary one hour before the commit on both 9.5.x and 10.0.x (https://www.drupal.org/pift-ci-job/2408203) it returned all green.

10.0.x HEAD comes back with the same 70 failures after the patch was reverted.

Something with the letters W, T and F springs to mind...

I can only conclude that this patch isn't the root cause of the HEAD failure.

Comment #30

21 June 2022 at 15:33

catch committed 8f72e9c on 10.0.x

Issue #3267314 by quietone, danflanagan8: Handle migration tests for...

Comment #31

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 21 June 2022 at 15:35

Since this doesn't look like the culprit, I've reverted the revert. However I also can't see an obvious candidate for the culprit either.

Comment #32

Spokje

Dutch

CreditAttribution: Spokje as a volunteer commented 23 June 2022 at 07:08

Status:

Needs review

» Fixed

I think this needs to go back to fixed since the revert was reverted and both 9.5.x and 10.0.x contain this code.

Comment #33

7 July 2022 at 07:09

Status:

Fixed

» Closed (fixed)

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

Handle migration tests for removing tracker

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Parent issue

Referenced by