Add content in migrate UI functional tests only when needed

This makes sense to me.

The patch for this issue needs a reroll after #3265424: Move migrate related aggregator tests to the module in preparation of removal in d10. I am adding the Novice tag for the reroll.

I'll work on this reroll

Here is the reroll for D9.4

@quietone:

I notice that the patch in #8 removes two @todo comments referring to #3267040: Remove @todo in migration functional tests. Can we close that issue as outdated?

@andregp:

I forgot to mention: there are separate patches in #4 for 9.4.x and 10.0.x. Can you reroll the 10.0.x version as well?

@benjifisher, sorry, I thought that as the D10 patch was still green it didn't need a reroll. I'll work on it.

Here's the reroll for D10 with a diff.

I'm also re-sending the same the patch file (for D9.4) from #8, just to keep everything in the same comment.

@andregp:

Thanks for the reroll!

I compared the 10.0.x patch in #14 to both the 9.4.x patch in #8 and the 10.0.x patch in #4. The reroll looks good to me, so back to RTBC.

I thought that as the D10 patch was still green it didn't need a reroll.

The testbot automatically re-tests patches (or MRs) for RTBC issues every two days, but it only runs one test per issue. Look for the little bot icon. The patch in #4 for 10.0.x was not being re-tested, so you saw the green from the one time it was tested.

@quietone:

I still wonder whether we can close #3267040: Remove @todo in migration functional tests since the patches in #10 and #14 remove the @todo comments.

This issue needs work. I think the reason is that Drupal's PHPStan rules just got stricter.

The patch in #10 uses CreateTestContentEntitiesTrait in two test classes. That trait defines installEntitySchemas(), which calls installEntitySchema() (singular). Even though the trait's method is never called, PHPStan complains if installEntitySchema() is not defined.

I am not sure what the right fix is. Here are some possible approaches:

1. Define installEntitySchema() in the test classes, either directly or indirectly. For example, they could extend a base class or use a trait that defines it.
2. Break up CreateTestContentEntitiesTrait into two (or more) more granular traits, and update every class that uses the trait.
3. Wrap the definition of installEntitySchemas() in something like if (method_exists(static::class, 'installEntitySchema')) { ... } (untested).

Hmmm, this is a toughie!

To get myself up to speed, here are some facts. There are four entries for this in the phpstan baseline file:

	-
			message: "#^Call to an undefined method Drupal\\\\Tests\\\\migrate_drupal_ui\\\\Functional\\\\CredentialFormTest\\:\\:installEntitySchema\\(\\)\\.$#"
			count: 8
			path: modules/migrate_drupal_ui/tests/src/Functional/CredentialFormTest.php

		-
			message: "#^Call to an undefined method Drupal\\\\Tests\\\\migrate_drupal_ui\\\\Functional\\\\MigrateUpgradeExecuteTestBase\\:\\:installEntitySchema\\(\\)\\.$#"
			count: 8
			path: modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php

		-
			message: "#^Call to an undefined method Drupal\\\\Tests\\\\migrate_drupal_ui\\\\Functional\\\\MigrateUpgradeFormStepsTest\\:\\:installEntitySchema\\(\\)\\.$#"
			count: 8
			path: modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeFormStepsTest.php

		-
			message: "#^Call to an undefined method Drupal\\\\Tests\\\\migrate_drupal_ui\\\\Functional\\\\MultilingualReviewPageTestBase\\:\\:installEntitySchema\\(\\)\\.$#"
			count: 8
			path: modules/migrate_drupal_ui/tests/src/Functional/MultilingualReviewPageTestBase.php

The problem is for Functional tests using the CreateTestContentEntitiesTrait. It's ok for the several Kernel tests that use the trait, since the method installEntitySchema method is defined on the KernelTestBase.

In the interest of scope management I would say let's just add similar entries for to the phpstan baseline for Drupal\Tests\migrate_drupal_ui\Functional\d6\IdConflictTest and Drupal\Tests\migrate_drupal_ui\Functional\d7\IdConflictTestand then fix all of these in a separate issue.

If that's not allowed, I'd say let's postpone this issue and make a separate issue for fixing the existing problems logged in the phpstan baseline. Then come back to this one.

I tried the approach suggested on #18. Let's see if it will pass the tests.

Either it didn't work or I did something wrong =/

I have attached a new patch for 10.0.x and an interdiff comparing it to #14 (skipping #19). I think the patch in #14 should still work for 9.4.x and 9.5.x.

@andregp, it looks as though you updated the class name in the message key but not the path key. Also, the existing entries in core/phpstan-basline.neon seem to be ordered by path, and I have tried to be consistent with that. I am not sure how entries are ordered for a given path.

@danflanagan8, thanks for pointing out the baseline file. I think the other references to installEntitySchema() were added in #3260928: Move more PHPStan global ignore patterns to baseline, so I am adding that as a related issue. From the issue summary of that issue:

The problem with the latter ones is that NEW code, failing the checks, can be added to the codebase. This wouldn't occur if there were baseline entries for the existing failures - those will stay until cleaned but no new failure could be introduced.

According to that, we should figure out how to fix the problem first, and postpone this issue for now, as you suggested at the end of #18. But I agree with your first suggestion (implemented in the patch here). Since this issue was already RTBC when #3259355: Always do a full phpstan analysis on DrupalCI was committed, I think it makes sense to make an exception for this issue.

It looks as though I re-uploaded the patch from #14. The testbot did not like it any better the second time.

Luckily, I still have the patch that I meant to upload. I am not going to update the file name.

One of the failing tests is a FunctionalJavascript test, and the other is #3283795: ComposerHooksTest is broken on latest DrupalCI PHP container. Back to NR.

> Since this issue was already RTBC when #3259355: Always do a full phpstan analysis on DrupalCI was committed, I think it makes sense to make an exception for this issue.

Nice sleuthing, @benjifisher. I agree with this thinking.

The patch in #14 was already at RTBC. The patch at #22 is identical except for the additions to the phpstan baseline. The failures on #22 are unrelated to this issue. Therefore, I feel comfortable setting this back to RTBC.

Also I'm removing the Novice and Needs reroll tags, which are both outdated.

Thanks, @danflanagan8!

I am adding a little to the justification for adding some PHPStan baseline rules.

Patch doesn't apply any more since 16 Jun 2022.

Rerolled patch #22, please review it.

Thanks for the patch, @mrinalini9. Unfortunately, custom commands failed for #28. Back to NW.

Reviewing is easier when a re-roll diff is included as well. Please see the docs for instructions: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

Thanks!

Re-doing the reroll of #22 with interdiff.

Reuploading both patches here for clarity. This is the 10.x patch from #30and the 9.4 patch from #8 Going to queue tests against 9.5 to see if it's still good.

This was already RTBC, and all it was was re-rolled, so back to RTBC.

Both patches (9.5.x and 10.0.x) need another reroll.

If we are lucky, then the testbot will automatically test against 10.0.x once we get this issue back to RTBC.

Added reroll of patches #31 on Drupal 9.5.x. and Drupal 10.0.x.

@ravi.shankar:

Thanks for the rerolls!.

It looks as though one of the tests for 9.5 needs to be adjusted. Back to NW for the failing test.

The 9.5 patch in #31 is the same as the patch from #8. I applied that to a commit on the 9.5.x branch from 2022-08-21 before #3293813: Move RDF-related tests to RDF module. Then I merged with the current HEAD of 9.5.x. The only difference from the patch in #34 was the entity counts for menu_link_content, which caused the test for #34 to fail.

Running the test locally, I see additional failures. I guess they are caused by recent changes (in the last week), perhaps moving additional tests. I adjusted the entity counts further to get the tests to pass, but maybe that deserves closer review.

I am attaching a new patch and an interdiff.

Edit: I think I selected 9.5.x when I uploaded the patch, but it looks like the testbot is using 10.0.x. If the patch does not apply, then I will try again.

It turns out that the additional failures I mentioned in #37 were caused by some untracked files in my local copy of the repository. So I am hiding the patch from #37 and attaching an interdiff comparing my new one to the patch in #34.

The first paragraph of #37 still applies.

For 10.0.x, the patch from #34 should still work.

My fairy godmother gave me permission:

You can RTBC your own reroll...
As long as you didn't write the code in the original patch.
And even then you are fine, as long as someone else RTBCed the original.

It looks as though I wrote the patch in #22, and @danflanagan8 reviewed it (RTBC) in #25. Since then, the only changes have been rerolls.

• 9.5.x: I compared the patch in #31 to my reroll in #39 (ignoring the bad rerolls in #34, #37). LGTM.
• 10.0.x: I compared the patch in #30 to the reroll in #34.

Both rerolls look right, so back to RTBC.

I am updating the target version to 10.1.x. I tested, and the patch still applies cleanly.

If we're going to do this we should avoid changing the PHPStan baseline by making the following change:

   /**
    * Install required entity schemas.
+   *
+   * This method is only useful for kernel tests.
    */
   protected function installEntitySchemas() {
-    $this->installEntitySchema('block_content');
-    $this->installEntitySchema('comment');
-    $this->installEntitySchema('file');
-    $this->installEntitySchema('menu_link_content');
-    $this->installEntitySchema('node');
-    $this->installEntitySchema('path_alias');
-    $this->installEntitySchema('taxonomy_term');
-    $this->installEntitySchema('user');
+    if (method_exists($this, 'installEntitySchema')) {
+      $this->installEntitySchema('block_content');
+      $this->installEntitySchema('comment');
+      $this->installEntitySchema('file');
+      $this->installEntitySchema('menu_link_content');
+      $this->installEntitySchema('node');
+      $this->installEntitySchema('path_alias');
+      $this->installEntitySchema('taxonomy_term');
+      $this->installEntitySchema('user');
+    }
   }

This will allow the trait to be used in both kernel tests and functional tests without affecting the baseline. We will need to clear up the baseline though so the D10 and D9 patches will still need to be different.

Re-roll patch created for 10.1.x, Please have a look.

Addressed comment #42, Please have a look.

@alexpott, thanks for the guidance.

@vsujeetkumar, the "Custom Commands Failed" status on the test means that static analysis (PHPStan, CSpell, or other) failed. Unfortunately, the testbot does not set the status to NW when this happens, so I am doing that now.

Looking at the testbot results, the problem may be that when you fixed the underlying problem, you did not remove the PHPStan baseline changes that we added in earlier patches.

Tried to fix CCF #44, attaching the interdiff for reference.

CC failure.

So many rerolls!

#53 looks to be passing though so hopefully last one.

Hopefully MR reroll will be the last

This looks good but I think we need to sort out some cruftiness around \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase::assertEmailsSent()

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.

Feedback appears to be addressed I believe. Unless we need to actually deprecate assertEmailsSent() but not sure with module specific test bases.

Hiding patches for clarity

The recent changes look good - like where the asserting no emails have been sent has been added. One thing - I think we should deprecate the method before removing it because it is on a testbase class so part of the test API.

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.

So many rerolls sorry but can we open up 2 MRs now

1 for deprecation into 10.3.x
1 with those removals for 11.x

Unfortunately, I can't push to the branch I just created for 10.3. Yes, it is 10.3 not 10.4 like that name suggests.

Don't follow the MR is pointed to 10.4?

Isn't there suppose to be a deprecation?

I added some comments to MR 7882 (targeting 10.4.x). Some or all may also apply to MR 7261.

@quietone, can you explain in the issue summary what should be done with the two PRs? In particular, how do we end up with the deprecation notice in the correct branch or branches?

Please also check the CR and the deprecation notice to make sure that they reference the correct branches.

Is it too late to deprecate this method and remove it in the 11.x branch? Is it even in scope for this issue? If so, then please explain in the issue description. Or maybe it is in scope, but too late to remove it in 11.x, in which case it might be simplest to defer it to a follow-up issue.

The issue summary says something about the changes to the PHPStan baseline. I think that needs to be updated, at least for the 10.4.x branch.